ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
858 stars 125 forks source link

fix: possible fix for hydration bug where it was killing text nodes o… #257

Closed i-Welch closed 9 months ago

i-Welch commented 1 year ago

Work Done

To Test

To Do

i-Welch commented 1 year ago

After doing some dissecting I can confirm that the issue is that the code running hydrate doesn't populate string and numbers into the elements it generates. From looking at the PR history this seems to be a performance optimization, but since the code also replaces the current Dom with the generated dom it causes certain strings and numbers to essentially just be disappeared until a signal is triggered to update that value in which case it returns (unless of course it's a static value in which case you're SOL)

i-Welch commented 1 year ago

I've tried to see if it's possible to just not insert the generated dom into root element but that seems to just break reactivity generally since the hydration code still attaches all reactivity to the generated nodes and not the existing nodes in the DOM

i-Welch commented 1 year ago

@ryansolid pinging this. This seems to be a pretty good fix at the cost of the performance improvement. If you don't think that's an acceptable outcome or I'm mistaken on what that code I removed actually does I'm open to spending more time here to try and find a different fix (which I think I have a pretty good idea what the fix will be but it's a bit more complicated) I'm good with that I'd just like a bit of movement in this.

ryansolid commented 1 year ago

Do you have an example closer to what you are after. Ideally we don't create DOM nodes during hydration. We're going in a direction where healing won't work long term so I'd rather find the cases where we are incorrectly creating DOM nodes rather than setting text nodes we should never need to set. I know there are some gaps here but I'd love to have more concrete example.

It is just that setting this back to correct the text still hides the more considerable underlying problem I think.

ryansolid commented 9 months ago

The examples here are creating new nodes rather than hydrating on the client (including in the test). Ie it isn't equivalent to what Solid's compiler generates. I don't see us merging this as the fix so I'm closing this. I need a better reproduction.