Skip to content

Add Mikado 0.8.1 #1534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 23, 2023
Merged

Add Mikado 0.8.1 #1534

merged 4 commits into from
Dec 23, 2023

Conversation

ts-thomas
Copy link
Contributor

A new generation of Mikado is finally ready to ship.

I removed Mikado from the "broken-frameworks" because everything is fine. I also remove issue tags, because they are no longer related.

For the keyed test I posted 2 versions. Let me shortly explain my purpose. Both tests differs by a maximun on their strategies. The "mikado" has a full declarative approach whereas the "mikado-proxy" test follows a full reactive approach. Look into the test implementation, the "mikado-proxy" just applies changes on an Array by using native Array methods and index access. It might look similar but that's just my style of coding and also the very limited test case. They are absolutely different in any way and I really would like to have a comparison of both strategies. Since I really maxed out both strategies, it is very interesting to have a comparison of same library, because here it just compares raw strategy. Also I won't suggest to use the proxy version by default. Personally I use the declarative way more often, but I also would like to present how good the performance is of a full reactive paradigm. It is a clash of the 2 fastest test implementation in your test framework. It feels really exciting.

Thanks a lot

@krausest krausest merged commit c6e6e9b into krausest:master Dec 23, 2023
@krausest
Copy link
Owner

Thanks. Here's a screenshot from the results (left new/updated keyed version, right old keyed version):
Screenshot 2023-12-23 at 4 22 57 PM
Results are updated.
The next full run will be exiting. It could be that recent security patches cause worse performance for recently updated implementations(i.e. vanillajs + mikado might have a slight disadvantage in the incremental results).

@ts-thomas
Copy link
Contributor Author

I'm really exciting about the new patches and how they impact. The most fun of all is to find something, which could be improved.
Thanks a lot

Comment on lines +28 to +30
const tmp = data[1];
view.replace(1, data[1] = data[998]);
view.replace(998, data[998] = tmp);
Copy link
Contributor

@leeoniya leeoniya Dec 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krausest @ts-thomas

should we have an additional flag for implementations like this?

i would not consider this type of implementation to be declarative, and is not too far from jQuery where you update the data and the view together to keep them in sync :) the view updates are not "driven by data", they're driven by imperative calls from events.

(same with "update" above, and "select" below)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's pretty close to the metal.
We could use #772 for that or invent a new flag, maybe something like "Implementation uses imperative code for reconciliation".
Your opinions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think either way is fine. changing #772 to say "imperative UI updates" feels better, just to avoid additional flags. but i suspect authors may want to distinguish between the strategies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reduce to what it is. It initialize an update by a numeric index. Therefore every library which uses a numeric index to initialize this update task needs to be flagged also. Then I'm fine with it.

For example from "million":

const swapRows = () => {
  if (list.length > 998) {
    const item = list[1];
    list[1] = list[998];
    list[998] = item;
  }
  update();
  return false;
};

Is also initializing this update by index. The only difference is when it's done, and this is a specification of the library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test has always been about testing a general reconciliation strategy (because that's what react does poorly). otherwise, the literal "swap" by itself is a pointless/stupid test. we can probably agree that it's stupid if it doesnt test anything meaningful?

we should just replace this with a seeded random "shuffle 100 data items into new positions", so that implementations which treat it as a "swap" test look obviously absurd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will replace it and make a pull request tomorrow.

I would like to explain why it isn't very useful when expecting a specific (almost data-driven) style.

Let's take the example from million. I don't know how the library is implemeneted, but I have a draft of a "transaction" feature for Mikado reactive proxy.

A look behind the scene is almost like:

// a task queue under the hood
const task = [];

// a proxified list
const item = list[1];
// the proxy trap do: task.push(update)
list[1] = list[998];
// the proxy trap do: task.push(update)
list[998] = item;

// execute the update by:
update();
// will run under the hood:
task.forEach(update => update())

Unrolled it just do:

update(task)
update(task)

wich is technically the same as:

update(index)
update(index)

The only difference is the specification of the library which just uses two different approaches, sharing the same complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test has always been about testing a general reconciliation strategy (because that's what react does poorly). otherwise, the literal "swap" by itself is a pointless/stupid test. we can probably agree that it's stupid if it doesnt test anything meaningful?

we should just replace this with a seeded random "shuffle 100 data items into new positions", so that implementations which treat it as a "swap" test look obviously absurd.

It's not the implementation, the problem is the nature of the test. A better concept is what I'm using in my benchmark, where the problem isn't known, because data is coming from an external authority. So the whole test implementation code reduces to something like:

suite["mikado"] = function(items){
    mikado.render(items);
};

Anything else is useless for me as a benchmark scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should just replace this with a seeded random "shuffle 100 data items into new positions", so that implementations which treat it as a "swap" test look obviously absurd.

Why hasn't this been used? Is there a discussion on it?


Mikado.once(document.body, tpl_app).eventCache = true;

// This implementation is using a full declarative paradigm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure this is true here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a declarative usage:

view.update(1, data);
view.add(data);
view.remove(data);

This isn't:

const data = [];
view.render(data);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants