-
Notifications
You must be signed in to change notification settings - Fork 45
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
Performance issue #60
Comments
This is unfortunately true... I started doing some benchmarks with js-framework-benchmark and noticed a particularly bad behaviour on bulk updates. That's the same issue as you observed. Indeed, some serious refactoring will be required, since this will definitely exceed the size budget 😞 |
Okay, so, just a thought, but I was able to remove all of this: Lines 76 to 95 in ddd6d50
And this: Lines 272 to 284 in ddd6d50
All without affecting the core functionality - which makes me think, the core library itself has no dependency on this, and JSX works fine without out, so does this have to be part of the core library? It seems you could push this to a plugin and give yourself a little more elbow room, so we don't trade-off performance for a few extra bytes saved? |
Yes, you are right. This is just implementing the Declarative DOM Composition extension and is not part of the core library. I haven't gone through the benchmarks yet, so I haven't really compared the final results of JSX versus declarative DOM. If JSX is a clean winner on the final size, I will consider removing it. However so far this:
Compresses more efficiently than this:
But on an actual project, the repetitive |
But yea, if this allows us to achieve better reconciliation performance, I will consider it. |
I'd expect the declarative DOM syntax would compress better, but someone (me) might still choose JSX perhaps mostly out of preference - so it would be nice if this API was optional either way. I mostly suggested it as a work-around to your painstaking 512 byte limit 😁 |
This may be slightly off-topic, but here's my unminified version so far, if you're interested. I dropped the proxy stuff and turned it into an ES6 module. Currently clocks in at 519 bytes, I'm sure that's completely unacceptable to you. 😜 |
It would be interesting to see how .dom stack up against other frameworks. I have previously used https://github.com/mathieuancelin/js-repaint-perfs (online demo here) myself to test a simple templating engine I built. |
I noticed that the
setState
callback currently updates not only the affected node, but all of it's siblings.dot-dom/src/dotdom.js
Line 134 in ddd6d50
State updates are synchronous, so imagine you have to update 10 sibling components - each component calls
setState
and triggers updates of all the other component, so a total of 10*10 = 100 updates.Since the function signature is
render(vnodes, dom)
, it looks like there's currently no way to update an individual child node.Likely this needs to be refactored, something like extracting a function
renderNode(vnode, domParent, domChild)
and calling that from thesetState
closure instead?The text was updated successfully, but these errors were encountered: