-
Notifications
You must be signed in to change notification settings - Fork 89
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
VNode reuse #32
Comments
Hi @tmadden,
The patching algorithm works similarly to the one of snabbdom, here you can find exactly your issue.
I think you can reuse nodes if you are sure you have the same tree shape, i.e. no additions or removals, same keys and types. For example, this should work fine: VNode* sharedVNode = <div>This is a shared VNode</div>;
VNode* vnode = (
<div>
<span style="font-weight: bold">Bold text</span>
{sharedVNode}
<a href="/foo">Link to foo</a>
</div>
);
patch(root, vnode);
VNode* newVnode = (
<div>
<span style="font-weight: normal">Normal text</span>
{sharedVNode}
<a href="/bar">Link to bar</a>
</div>
);
patch(vnode, newVnode); Please let me know if you have any other question or alternative solutions, so we can discuss it further and improve our libraries together 😄 |
Ah, OK, thanks for the explanation! It could be that what I really want is just a plain old object-oriented C++ interface to the DOM so that I can manually add and remove nodes when needed from my library. I'll close this for now and just avoid reusing VNodes. As I gain more experience using the two libraries in real applications, I'll see what I learn in terms of whether or not this is ever a real performance problem and whether or not there's anything that should be done about it. Thanks again, and thanks for the kind words about my library. I'd love to see the two grow together. :-) |
Sure! Let me know as soon as you can so we can improve them. |
Hey, I wanted to check back in on this. Things are slowly progressing on alia. I'm starting to use it (along with asm-dom) to develop some small in-house web apps. I had a chance to do some basic profiling, and it turns out that the use of asm-dom's virtual DOM ends up having a pretty large performance impact on my test UIs. This isn't necessarily asm-dom's fault. It's more a mismatch between the two libraries, and I think the biggest factor is just that I can't skip over the application code for large chunks of UI even when I knew there are no changes. alia does its own diffing, so right now I've hacked up an integration between asm-dom and alia where alia calls into asm-dom's internals to directly create nodes and apply patches. It's mostly working, but there are definitely some bugs, so I'm interested in a 'proper' solution with an API and test cases and such. So basically what I'm interested in would be an object-oriented C++ API to the DOM that looks a lot like the browser's actual JS DOM API. As far as I know, there are no libraries out there that currently provide this, and asm-dom is the closest. You basically already have the code for this, just not directly exposed as a C++ API. Would you be interested in collaborating on adding this? I think it could even be another layer in between the existing asm-dom virtual DOM and your JS code. (i.e., You could refactor the virtual DOM diffing/patching in terms of the lower-level object-oriented API, but obviously this might be more complicated than I realize.) I think having an API on this level would also open the door to an improved debugging experience. Maybe you have some tips, but I've found the Emscripten debugging experience to be pretty lacking compared with native debugging of C++ desktop apps. What I'd ideally like is to have a small proxy running in the browser that's independent of any application logic and then (during development) implement the actual application logic in native C++ and have that drive the proxy. (The API could be the same as this "object-oriented asm-dom", but with a WebSocket layer in between for development purposes.) Maybe there is even some potential here for doing native C++ apps that use webviews for their UI (as ironic/infuriating as that might be :-p). Anyway, that last part is obviously further down the road, but I think the first step of creating this object-oriented C++ DOM API is doable, so I just wanted to reach out to see what your thoughts are. Specifically:
Also interested to hear any other thoughts you have on all this! |
Hi @tmadden,
Are you using the internal of asm-dom only in the following file? (So the js calls?)
Could you please try to report here such bugs? I'll reply here with a proper analysis of the situation, also answering your two questions, either tonight or tomorrow :) |
Hi @mbasso! Thanks for the response! :-) Yeah, that's the code. I think there is some stuff in the accompanying header file too. I work on that code outside of that repository as well (as I work on the actual web apps). I recently started a dedicated repository for it here: https://github.com/tmadden/alia-html However, I think the code that integrates with asm-dom is the same as what you found, so there's really no need to review both. I think you're correct that I really only use the JS code (because the C++ code doesn't present the style of API that I need). As far as the bugs go, I don't think they are necessarily bugs in your code, but they are bugs in the sense that the web apps I write aren't behaving as they should in all circumstances. But there are a few layers involved here:
So basically 1 and 6 don't always agree, and it's pretty clear that the problem is somewhere in between. I think it's probably in level 4 (or maybe level 3), but it's not easy to isolate. Obviously, with something like this, it's easiest to get a handle on things if you isolate the layers and test them individually, so that's essentially what I'm getting at above. I'd like to do layer 4 properly, with unit tests. I could do it internally within my project, but it might be useful outside of that. |
@tmadden I am subscribed as "watcher" and I am happy that you are developing such a great library, keep it up :)
Performing the diffing twice can definitely be costly. To solve this issue you can #include "asm-dom.hpp"
using namespace asmdom;
int main() {
Config config = Config();
// we need to free allocated VNodes after patch calls
config.clearMemory = false;
config.unsafePatch = true;
init(config);
VNode* link = <a href="/foo">I'll take you places!</a>;
VNode* root = (
<div
onclick={[](emscripten::val e) -> bool {
emscripten::val::global("console").call<void>("log", emscripten::val("clicked"));
return true;
}}
>
<span style="font-weight: bold">This is bold</span>
and this is just normal text
{link}
</div>
);
// patch the whole tree structure
patch(
emscripten::val::global("document").call<emscripten::val>(
"getElementById",
std::string("root")
),
root
);
// new link with a different href
VNode* newLink = <a href="/bar">I'll take you places!</a>;
// patch only the link
patch(link, newLink);
//
// here you would need to replace the link in the old VNode tree with the new one
// (iterating over VNode.children or even better specializing each time for the specific case).
// implementation omitted for brevity
//
// replaces oldVNode with newVNode in the given tree
// replaceVNode(VNode* tree, VNode* oldVNode, VNode* newVNode)
//
replaceVNode(root, link, newLink);
// at this point "root" represents the new updated tree you can reuse
// since clearMemory is equal to false, we need to manually free the allocated memory that we don't need anymore.
// Please refer to the API introduced in [email protected]
// https://github.com/mbasso/asm-dom/releases/tag/0.6.0
deleteVNode(link);
return 0;
}; Please notice that if you want to change, for example, a single attribute of the root element, you can do something similar to what I showed in my previous code snippet (comment of March 29). This might theoretically work but in practice, it might fail under certain circumstances since VNodes are not designed to be modified once created. Given so, we can think about an API to force a VNode normalization even after its creation. In this way, I think you should be able to call patch multiple times reusing VNodes. Please notice that also introducing this API, using the same VNode to map multiple real nodes is not possible. For example, the following does not represent valid code: VNode* link = <a href="/foo">I'll take you places!</a>;
// using the same vnode (the same link) multiple times does not work
// since a single vnode maps a single real DOM node.
// If needed, you would need to create exactly the same instance twice using a factory function
VNode* root = (
<div>
{link}
{link}
</div>
);
If you want to directly modify the DOM without calling Regarding the "object-oriented C++ API to the DOM", I would do something slightly different. I would leave the void setAttribute(VNode* vnode, std::string attribute, std::string value); Its behavior differs based on the provided vnode:
This means that you don't even need to worry about VNode normalization. Obviously, you need to store your VNodes instead of just the Other examples can be the following: void setAttribute(VNode* vnode, std::string attribute, std::string value);
void removeAttribute(VNode* vnode, std::string attribute);
void appendChild(VNode* parent, VNode* child);
void removeChild(VNode* vnode);
void setText(VNode* vnode, std::string text);
void setComment(VNode* vnode, std::string comment); In addition, we need to add some other APIs that are not direclty mapped by void setProperty(VNode* vnode, std::string property, std::string value);
void removeProperty(VNode* vnode, std::string property);
void setCallback(VNode* vnode, std::string event, Callback callback);
void removeCallback(VNode* vnode, std::string event); What do you think about it?
As you might have understood from my explanation, it makes sense to add such API directly into asm-dom. The JS-WASM communication is not easy and separating the two things might be problematic. It would mean using asm-dom in a different way, not as a pure virtual-dom. So I am not sure whether I should include the new API in a different namespace, for example At this point, someone can even consider using asm-dom without even calling // previously defined function
void appendChild(VNode* parent, VNode* child);
// potential additional versions that allows you to avoid patch
// since they take DOM nodes instead of virtual ones
void appendChild(emscripten::val& parent, VNode* child);
// are the following really useful?
void appendChild(emscripten::val& parent, emscripten::val& child);
void appendChild(VNode* parent, emscripten::val& child); What do you think?
Of course, I can update asm-dom to implement these changes :) (or anyway the API you would like to have) P.S. I can see that you are reading |
Thanks so much for your encouragement and support! I really appreciate it! :-)
Yeah, I think I could probably get the original asm-dom API to work for me in most cases. There are still cases (like I mentioned in my original comment) where asm-dom tries to overwrite a node that alia thinks it can reuse. I could potentially detect cases where this is going to happen and regenerate the nodes that are going to cause problems. Ultimately though, it just seems like extra work (not just developer work, but extra processing) for no real benefit. alia is designed to be able to directly detect exactly what needs to be done to update the UI object tree (i.e., the DOM), so it can directly issue commands like "Create a new text node and insert it over here", "Set property ABC of widget W to XYZ", or "Move this whole div before this other div." This is why I'm hoping to just be able to issue commands in the same form "directly" to the DOM. By the way, this is not just a concern about asm-dom doing unnecessary work (unnecessarily creating VNodes and diffing them). Even if that is essentially free, the alia application would have to do unnecessary work to regenerate the VNodes, which might not be trivial. I use alia to create fairly complex medical imaging applications, so in my experience (on the desktop side), it's best to minimize this.
I'm not sure I entirely follow all your points about patching, but I'm pretty confident that my issues are not a result of synchronization between what I'm doing and VNode patching/normalization. I don't think I'm actually even using any VNodes (or calling Which brings me to this part of your comment...
I think you are operating under the assumption that an application using this new API should be able to freely mix it with the current asm-dom API. In my mind, they would be two entirely separate levels of API, and an application would either use one of the other. In other words, either I'm going to trust asm-dom to do all the diffing/patching itself (in which case I use the existing And like I said, the
I would take your stack and replace the diffing/patching engine:
And maybe someone else would even come up with yet another diffing/patching engine. Does that make sense? Maybe I'm missing something, but other than the My initial thought was that we could try to make the lower-level API as close as possible to the native JS DOM API provided by the browser (but not try to make it a complete replacement). So for example, we could have a So I guess I'm thinking of what you're talking about here...
So I think it is useful to have something at that level, but maybe using What do you think?
Thanks again! I really appreciate it! :-)
Yeah, I noticed this, so I just froze it at an earlier version of asm-dom. I decided it would be better to follow up with you to pursue a proper solution than to try to keep my hacks up-to-date. ;-) |
It makes sense to me. I can proceed with implementing such API (the one I proposed but using
This needs to be well explained in the documentation. We would also need to change the description of asm-dom and its statement. What do you think?
Yeah, this should increase performance, passing raw |
Sounds great! :-) I think it's a good idea to keep it unofficial for now and to shelter the official asmdom API from this work, at least until it's more mature. Whatever separation you think is appropriate is fine with me (separate branch, repository, whatever). And I'm happy to help however I can. |
Perfect! :) I am currently updating the internals and I am facing some tricky issues. I will let you know as soon as I can. |
Well that was fast! :-) The API looks great! I can't think of anything that I need that's missing from the list. I was about to say that there might be some functions in the list that I don't need, like these: inline int parentNode(const int elm);
inline int firstChild(const int elm);
inline int nextSibling(const int elm); But as I think about it more, I realize those could actually be helpful for debugging, so if they're not a lot of work, I vote to leave them in. :-) Is that branch in a state that I could try using it? I should have some time later this week to work on this, so I could try getting rid of my hacks and using the new API! :-D That would help confirm that it has all the functions I need. Thanks again! |
Thank you :)
Yeah, they are needed to navigate the DOM tree without using
Sure, you can clone it without any problem (I have not published it on npm yet). Just a couple of notes:
Given so, to remove and delete and element you would need to code something like this: const int elm = createElement("div");
appendChild(root, elm);
// later in the code
// remove elm from the DOM tree
remove(elm);
// once removed, free memory
deleteElement(elm);
const int elm = createElement("div");
assert(parentNode(elm) == 0);
assert(firstChild(elm) == 0);
assert(nextSibling(elm) == 0);
const int child1 = createElement("div");
appendChild(elm, child1);
assert(firstChild(elm) == child1);
const int child2 = createElement("div");
appendChild(elm, child2);
assert(nextSibling(child1) == child2);
assert(nextSibling(nextSibling(child1)) == 0); Let me know if everything is ok :) |
OK, so real work got in the way last week, but I've been working on integrating the new API today. There's still a lot of work to be done on my side, but it is mostly working in my little sandbox. I did encounter one issue though, with this function: void setProperty(const int elm, const char* property, const char* value); Unless I'm misunderstanding things, it seems that properties can be other JS types besides strings. (I've been using booleans to set the checked state of checkboxes, for example.) Should this take the value as an |
Yes, I have updated the API :) |
Sweet, thanks! Checkboxes are working again! :-) |
Hey, I just wanted to follow up on this. I haven't had much time to work on alia lately, but I did do some testing in my example apps, and using the new API seems to have fixed the issues I was having with my hacks! :-) (It seems that I was having some issues with double-deleting elements. (Because the parent element would get deleted first, and then I would assume that the children also had to be deleted, when in fact asm-dom had already deleted them.) Having the new API made it much clearer what was going on.) So I think from my perspective, this issue is resolved. Thank you! Should I close it, or do you want to wait until the new API is merged in? Anything I can do to help with that? |
I am happy to hear that!
Cool!
I think we only need to write an example, maybe a simple one (not the full TODO MVC), and the documentation. |
Yeah, apparently I don't have a lot of time these days. :-p I think it would actually be fun though to contribute. Maybe I could do the example? I don't think that would take much time. Would a port of the "simple counter" example in the documentation be appropriate? I think that would be relatively straightforward and would show off a good chunk of the API. |
I am sorry for my late reply but I have been so busy in this period...
That would be great :)
I agree. What about implementing the Counter example of the boilerplate (https://github.com/mbasso/asm-dom-boilerplate)? |
No worries! You're not the only one. :-) Happy New Year!
Sounds good! And it will force me to learn a tiny bit on how to use node, parcel, etc. :-p How would you want to integrate it (or not)? Like would I just create a separate repository (asm-dom-direct-boilerplate?) that mimics that repository but uses the direct interface? |
Thank you! Happy New Year! :)
I think we can create and place the example in the You can use either webpack or parcel, I have no strong preference, i.e., you can reuse the boilerplate or you can reuse the structure of another cpp example. What do you think? :) |
Sounds good. Thanks for the clarification. I'll follow up if I have any other questions. :-) |
General Information
Depends on the answer to my question I suppose. ;-)
Description
Hi, awesome library! :-) I've been developing a declarative UI library, and hooking it up to asm-dom has enabled me to create live examples of it in the documentation:
https://tmadden.github.io/alia
I've been considering creating a production-grade interface to asm-dom, but I have a question about doing this properly... My own library provides the mechanics for tracking whether portions of the UI specification have changed since the last update. Ideally, when I know that an element hasn't changed, I'd like to reuse the VNode from before rather than recreating it with
h()
. However, it seems that asm-dom does some subtle things with VNodes during patching that make this complicated. I thought I had this working at one point, but then when I reordered my nodes within their parent container, things broke. (Some of the nodes had apparently overwritten others in the resulting UI.)Do you have any guidance on this? Is it a supported use case? Are there rules one could follow to at least reuse some nodes safely?
Thanks!
Versions
The text was updated successfully, but these errors were encountered: