Skip to content
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

upgrade xaniajs implementation to 0.3.* #991

Merged
merged 3 commits into from
Jan 22, 2022
Merged

Conversation

xania
Copy link
Contributor

@xania xania commented Jan 17, 2022

No description provided.

@xania
Copy link
Contributor Author

xania commented Jan 18, 2022

this update should "hopefully" prove to be even more performant than the previous submission of xaniajs

@krausest krausest merged commit 0f0234e into krausest:master Jan 22, 2022
@krausest
Copy link
Owner

Thanks. Xaniajs got really fast!
But I'm afraid I think I have to add the #772 marker for the implementation due to setting classname directly and removing rows directly. I hope you agree on this finding.
Anyways: Impressive results!

@xania
Copy link
Contributor Author

xania commented Jan 22, 2022

Hi, thx again, I don't think I agree with setting className directly, updateAt uses completly generic (optimized way) to update the dom. Under the hood we use the specified index to find the pair of dom element and corresponding row data which is then updated simply by setting its property to the provided value and then the compiled dom operations (derived from jsx template) are applied on the dom element with row data as input.

However, removing rows directly is actually valid point, but I dont see why that is bad thing (assuming a marker is bad), I think I need to dig into description of the marker but my first thought is, direct removing of the rows is actually a result of a special property of the xaniajs framework. xaniajs does not create any explicit bindings or subscriptions or listeners or any node specific mappings which means there is nothing we need to keep track off when dealing with specific node, not during creation nor during deletion.

@xania
Copy link
Contributor Author

xania commented Jan 22, 2022

BTW, className is a reference to the Property of the DataRow type and not a reference to the DOM node

export interface DataRow {
  id: number;
  label: string;
  className?: string;
}

and is used in the jsx template

function Row(store: TableStore) {
  return (
    <tr class={property("className")}>
      <td class="col-md-1">{property("id")}</td>
      ....

@xania
Copy link
Contributor Author

xania commented Jan 23, 2022

In the same category of confusion, the variable name container is not a reference to the DOM element itself, but its actually an abstraction of the list component, and it owns the data, any data changes need to go thru the list / container component.

@ryansolid
Copy link
Contributor

ryansolid commented Jan 23, 2022

I think the delete is fine. It's basically like manual event delegation being done in others but done more cleanly. Instead of walking to find the row Xania already wrote it to the element which you'd have anyway in any event handler and typically you'd have the index as well (the data being read). This basically avoids the closure like any of the methods used under the hood in other frameworks for implicit event delegation. It's more in the open here but it'd be hard to say it is #772. Maybe #801 but even that's arguable.

One thing you are doing is storing the tr in the selected state in the implementation code which qualifies for 772. I actually did the same thing when I first submitted Solid and didn't think twice about it. But was quickly directed to remedy that.It's definitely considered #772 now.

className on the model is considered #800. I understand why it's there mind you since you are basically proxying (not literally a ES proxy) over the DOM similar to Mikado and Sinuous' template. If I understand correctly all updates happen against this wrapping container that is one for one with DOM elements.

Overall this implementation is like Mikado in that it uses swap to proxy swap. There is no reconciler here. Replace clears before it attaches because it knows it is a replacement instead of diffing. It's this thread all over again: #654. I don't even remember if we landed anywhere on considering that #772 because the author changed his implementation to be more data-driven afterwards because he was tired of catching flack from people. @krausest I mentioned this in other issues I was surprised we hadn't seen more libraries like this. It's basically a different sort of model that works really well in these sorts of benchmarks which don't need to handle shared state synchronization.

@krausest
Copy link
Owner

Yes, this is really always a difficult topic. This benchmark certainly has a weakness in that it allows a comparison for all kinds of frameworks (we only managed to distinguish between keyed and non-keyed). More imperative frameworks often have an edge over model-driven benchmarks, but the lines between the frameworks are blurry.
We tried to handle that situation by adding those notes. This should not say that such a framework it's bad in any way. As one can see it can be readable and well performing.

@xania
Copy link
Contributor Author

xania commented Jan 24, 2022

thx guys for the impressive in-depth analysis of xaniajs' implementation, it even helped me understand it better myself :)
I guess the objective of this benchmark is mostly intended toward the model-driven approach, and it is totally a valid choice to make, but isnt that just implementation detail we sould not care about?
The model-driven approach has in my opinion a lot of drawbacks and unnecessary complexity, most notable is that xaniajs does not need a reconciler to detect the changes , typically we load data from server and from that point we own the data so for me it doesnt make sense in most cases not to communicate e.g. which rows are swapped to the framework just to force it towards reconciliation. But if the component does not own data then reconciler could be added on top of the already efficient direct approach.

And if you really want to force the model-driven approach then I would suggest to provide the data as external source
e.g. make the ts-driver generate data and then call into some global api provided as global on the window object

window.store.init(data)` 
// update data
window.store.sync();

@ryansolid
Copy link
Contributor

ryansolid commented Jan 24, 2022

Yeah and to be clear reactive frameworks are sort of teetering on that line since they also follow the thing that once loaded you own the data. Pulling it outside and doing snapshots would put them out too or relying on sneaking proxies in because they too need custom data. The difference between Xania's approach and reactive libs is just what is being proxied. Reactive libraries intercept the data updates, but work off multi-cast events so the data to template relationship isn't 1 to 1. Neither need a reconciler in the pure sense.

So I don't think the intention is to try to force this into basically a VDOM only thing. However, I do see the dilemma. Replace and Select rows are sort of the perfect examples of where this gets tricky. Any framework could clear before insert for their replace. Or benefit from putting selected on each row. The reason for the different tests are these super common with model-driven approach. If selected was on each row it is basically the same as partial updates test. Often with model-driven approach more than one template can represent the same data so polluting the model is a case to avoid. As are specific data projections where you preserve the source list but need to show filtered results and between changing those filters some of the elements may be retained and others not. So the tests are written in the spirit of those cases without actually being forced into them.

But if the component does not own data then reconciler could be added on top of the already efficient direct approach.

Yup. And the most efficient direct approach would just be using VanillaJS. So if the premise was the reconciler was necessary you'd just skip straight past proxying the DOM and use the DOM directly. I'm not saying it is necessary here (or anywhere else) as clearly Xania implementations shows but the abstraction difference does sidestep some of the tests.

View the notes as an indicator of that different behavior. I can picture how Xania could partially conform but as I told the author of Mikado might not be worth it, not a true representation of how one idiomatically uses the library. It's a choice. With Solid I was like 90% conformance so I just went slightly out of the way to do so, so I could remove the markers. But I think one of the strengths here is that you can just leave it as is and still showcase the library for the way it was meant to behave.

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.

3 participants