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

Add ScarletsFrame #519

Merged
merged 6 commits into from
Feb 18, 2019
Merged

Add ScarletsFrame #519

merged 6 commits into from
Feb 18, 2019

Conversation

StefansArya
Copy link
Contributor

Hi, I have added ScarletsFrame.

It's support array data binding,
where any array manipulation will cause change to related element.

Actually it also support virtual list too.
But I think it's a bit cheating to use that feature, so I'll not implement it 🤣

@Freak613
Copy link
Contributor

https://github.com/krausest/js-framework-benchmark/pull/519/files#diff-cd60a0dd3cfbfb96d9b0c9a07d313af8R93

We have a long discussion over should we perform direct dom manipulation from the data layer or not. And start considering hacks like this as dirty.

And we decided that view should, at least, be reconstructable from the data only. Having this implementation, the view won't be reconstructed if we provide initial value for selected

It's on your conscience, but it's better to move className binding to the view layer and perform direct modifications there (like Surplus). Or wrap it in some library helper (like Solid). Or whatever optimization you choose.

The fact is that this type of implementation can be easily ported to all other libraries, even in the slowest one, and we all start having great numbers here, but it makes this test meaningless.

Feel free to discuss this point in the mentioned issue.

@StefansArya
Copy link
Contributor Author

You're right, if we do a direct dom manipulation it would be a meaningless test for the framework.
I was forgot that one because trying to do similar implementation on the Vanilla JS..

select(idx) {
startMeasure("select");
this.unselect();
this.store.select(this.data[idx].id);
this.selectedRow = this.rows[idx];
this.selectedRow.className = "danger";
stopMeasure();
}

I will change the implementation for the test

@ryansolid
Copy link
Contributor

Yeah that was my case too. I had spent so much time optimizing for the vanillajs case that I hadn't even thought much of it.

Another optimization we've seen previously is to update the underlying model to have selected per row for libraries based on immutable data structures. That way immutability checks can skip all the rows except the newly selected and unselected rows during reconciliation. I'm unsure of what the opinion is on that approach.

I think in general the selected test is by far the most controversial test as there are fairly strong opinions on how to handle events that continues right into whether direct DOM manipulation during event handling is acceptable. There is a party that believes the DOM should be completely abstracted, that Explicit Event Delegation is an Anti-Pattern, and that Synthetic Events are generally a good thing. Which stands pretty opposite to the position that (especially with Web Components) the DOM is what you are dealing with anyway, and that Implicit Event Delegation and Synthetic Events can break interoptibility.

The only thing that could be agreed upon is that state at rest should be able to always reproduce the view. There is a suggestion that using events to expose the DOM structure to the implementor is a bit of a hack, but you can see how it become just arguing semantics if a library would do the same thing under the hood anyway.

@leeoniya
Copy link
Contributor

Another optimization we've seen previously is to update the underlying model to have selected per row for libraries based on immutable data structures.

yeah, this is a thorny one. in my view, shoving "selected" into each item is kinda polluting the models/data with presentation-layer state. on the other hand, it's typical in React-land to pass composed objects into views that mix all sorts of stuff, like callbacks, etc. this strategy certainly makes the diffing much faster for the immutable case. most impls adhere to keeping "selected" out of the items.

immutability is another one of those optimizations that is possible and would benefit many impls by reducing diffing overhead in this massive stress test of a benchmark.

@StefansArya
Copy link
Contributor Author

yeah, this is a thorny one. in my view, shoving "selected" into each item is kinda polluting the models/data with presentation-layer state.

Agreed, it would be better to save the element reference and the index of the selected item because I wouldn't want to iterating the whole array just for searching the selected item.

But the purpose of the keyed test is when the item was selected, the element will be replaced with a new element..

When I was trying to reproduce the VanillaJS implementation with the framework, I found that VanillaJS was doing a direct DOM manipulation and not rebuild the element... So I'm doing the same thing..

@krausest krausest merged commit 78b34d5 into krausest:master Feb 18, 2019
@krausest
Copy link
Owner

It's merged - results have been updated. I noticed a problem with the test driver. It hangs after the first iteration. The only quick, but very ugly fix was to add a sleep call to the test driver only for ScarletsFrame.
I guess this is because the template for the row is visible - do you think you can change your implementation such that the template is never displayed? Here's a thumbnail from chrome (sorry it's not very sharp)...

bildschirmfoto 2019-02-18 um 20 43 02

@StefansArya
Copy link
Contributor Author

Thanks, looks like I need to do more optimization before release it to production.

Actually the framework was setup a queue class on that template element, so we can build a skeleton loading with CSS.

But I suppose that the template should be removed before DOMContentLoaded is being triggered.
I think I know how to remove it just before being rendered on FCP, or before window.load event.

I will submit PR again after some version update 👍

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.

5 participants