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

hydration footguns #44

Closed
leeoniya opened this issue Mar 24, 2023 · 13 comments
Closed

hydration footguns #44

leeoniya opened this issue Mar 24, 2023 · 13 comments

Comments

@leeoniya
Copy link
Contributor

leeoniya commented Mar 24, 2023

the two i've run into are implicit tbody insertion (and couple other tags), and adjacent textNode fusion. the former can probably be solved by the htm compiler. the latter was never a problem with DOMVM since it does a vnode array normalization pass that also fuses. but ivi doesnt fuse and a text+expr may result in two text nodes. so i think you'll need to insert comment nodes for SSR and either leave them in or strip them out during hydration.

@localvoid
Copy link
Owner

localvoid commented Mar 24, 2023

the former can probably be solved by the htm compiler

It will be impossible to solve with template compilation, except for some basic cases when this violations will be detectable in the same template. It can be detected when entire tree is generated in development mode, or maybe it would be even better to just run through an html validator in development mode. I've seen issues in Next/React when people were complaining about this types of bugs some time ago, maybe they are better now at reporting this types of bugs.

Right now I am planning to add integration with Astro, maybe they're already detecting this types of bugs. Haven't checked it yet.

text+expr may result in two text nodes. so i think you'll need to insert comment nodes for SSR and either leave them in or strip them out during hydration.

Yes, even without SSR, templates are generating comment nodes:

if ((state & 3) === 3) {
staticTemplate.push("<!>");
}

But for SSR, there are even more cases when comment nodes should be injected and it will be possible to figure out when comment nodes are necessary only at runtime.


Also, to deal with some edge cases there will be an attribute added to some elements that contains some additional information for hydration, like <div &="1 2 3"></div>. The & character is a valid attribute name, and I hope that nobody is using it :)

@localvoid
Copy link
Owner

localvoid commented Mar 24, 2023

Just got another idea :) Right now, when templates are mounted, there is a code path

} else { // Remove
// Remove operation implies that current node is always a comment node
// followed by a text node.
const commentNode = currentNode as Comment;
state[++ctx.si] = currentNode = nodeGetNextSibling.call(currentNode);
commentNode.remove();
that deals with this text+expr edge case and removes a comment node for each cloned instance, but it would be better to remove it once when template blueprint is instantiated with innerHTML="static-template".

@localvoid
Copy link
Owner

And another thing that I haven't decided yet is that I want to produce different VNode objects depending on the execution context. For example, in ssr context List(entries, getKey, render) function will be a simple entries.map(render). And to make a public API sound, it will be necessary to declare that VNode is an opaque type.

@leeoniya
Copy link
Contributor Author

i find these two statrments a bit contradictory. yes i see one is talking about conditional markers vs adjacent text nodes, but expr must technically be treated as conditional also, so not sure:

Yes, even without SSR, templates are generating comment nodes:

Majority of libraries are dealing with this edge cases by introducing marker DOM nodes (comment or an empty text node). For example, to implement conditional expressions we can add an empty text node when conditional doesn't render any DOM node and when conditional goes into a state when it needs to add a DOM node, it will use a marker node as a next DOM node reference. In some edge case scenarios, some libraries can sveltejs/svelte#3586 of marker nodes. Update algorithm in ivi doesn't use any marker nodes.

@localvoid
Copy link
Owner

localvoid commented Mar 24, 2023

Yes, I agree that it is a little bit contradictory. Before template cloning, ivi didn't use any comment nodes, and with template cloning it is used only in an edge case when there is a "static text, expression, static text", and in the final document tree there aren't any comment nodes. Update algorithm doesn't rely on marker nodes to figure out the location for DOM updates.

In theory, I can replace comment node with encoding static string length into a StateOpCode and when it is "hydrated", split text node into two nodes. Not sure which workaround is better :)

@leeoniya
Copy link
Contributor Author

leeoniya commented Mar 24, 2023

In theory, I can replace comment node with encoding static string length into a StateOpCode and when it is "hydrated", split text node into two nodes. Not sure which workaround is better :)

imo the string length variant is better since it leaves the dom cleaner and in the expected shape (when inspecting in DevTools). but if you need the comment node variant anyways for some additional edge cases then maybe not worth having two codepaths?

@localvoid
Copy link
Owner

Right now, comments are only used outside of a document tree to deal with just one edge case:

htm`<div>text${expr}text</div>`

When template compiler sees two adjacent static text nodes separated by an expression, it injects a comment node to separate static text nodes <div>text<!>text</div>, then it renders lazily template blueprint with an innerHTML. And when template is created from a blueprint with a cloneNode(), comment node is removed before it is inserted into the document, so the final document is always clean.

Right now, template blueprint contains a comment node and it is removed in each cloned instance, but it is possible to remove it just once, immediately after blueprint is created with innerHTML.

Other than that, comment nodes aren't used. Except for an SSR, where as you pointed out in your first comment, it will be necessary to detect at runtime edge cases like: [static text, dynamic text], [dynamic text, static text] and [dynamic text, dynamic text]. It is possible to remove comment nodes after hydration, because they will be completely ignored after hydration, but I am not sure that it is worth it and maybe it would be better to just leave them to improve hydration performance. Also, it is possible to add an optional comment node garbage collector in an idle callback after hydration.

@localvoid
Copy link
Owner

localvoid commented Apr 2, 2023

SSR/Hydration required some major changes to the current API: public API surface is significantly reduced, stateful/stateless tree node types should be considered as a private(opaque) API, all client-side API is now imported from the ivi module instead of many different modules.

In SSR Context:

  • useUnmount() throws exception
  • invalidate() throws exception
  • useState() setter throws exception
  • useReducer() dispatch throws exception
  • createRoot() throws exception
  • dirtyCheck() throws exception
  • update() throws exception
  • unmount() throws exception
  • hydrate() throws exception
  • emit() throws exception
  • findDOMNode() throws exception
  • containsDOMElement() throws exception
  • hasDOMElement() throws exception
  • useEffect() returns NOOP
  • useLayoutEffect() returns NOOP
  • useIdleEffect() returns NOOP
  • useMemo() doesn't use any memoization useMemo = (fn) => fn.

Code that uses low-level APIs like useUmount() should use something like import.meta.env.SSR to avoid invoking it during SSR.

SSR Template compiler removes all unused expressions like event handlers from templates and rollup should be able to eliminate dead code and completely remove event handlers from components. In theory it is also possible to perform some basic static analysis in components and convert some stateful components into simple functions for SSR.

@leeoniya
Copy link
Contributor Author

leeoniya commented Apr 2, 2023

not sure i fully understand what throwing an exception means in practice. how does this not break SSR unless the runtime/compiler wraps this in try/catch and noops it anyway? why not just noop?

end-to-end example / docs might help.

@localvoid
Copy link
Owner

If one of this functions is invoked during SSR, it is most likely a bug in the code. For example, it is safe to return a NOOP function in useEffect(), because side effect will be completely eliminated during SSR. But if there is a code that subscribes to some event and uses useUnmount() to unsubscribe, it is a bug, and it shouldn't be subscribing in the first place. This code should be wrapped with if (!import.meta.env.SSR) { /* subscribe, useUnmount */ }.

It is possible to relax some constraints in the future without introducing breaking changes, but it would be better to just avoid using this functions during SSR.

@leeoniya
Copy link
Contributor Author

leeoniya commented Apr 2, 2023

but it would be better to just avoid using this functions during SSR.

are you saying the SSR code is not 1:1 with the frontend code unless it has all the necessary conditional SSR gating? 🤔

again, example would help :)

@localvoid
Copy link
Owner

are you saying the SSR code is not 1:1 with the frontend code unless it has all the necessary conditional SSR gating?

It is the same as with any other framework that doesn't use exactly the same code for client-side and server-side rendering and serializes something like JSDOM to string. There is no DOM (any DOM utils function invocation is a bug), components doesn't have a "lifecycle" (allocating resources without deallocation is a bug), etc. Proactively throwing exceptions in this functions just makes it easier to catch bugs.

@leeoniya
Copy link
Contributor Author

leeoniya commented Apr 2, 2023

i guess my view of SSR code is through the lens of domvm's implementation which works with no changes in both DOM and SSR contexts. i dont have experience with how it's typically done in other frameworks, so i'm sure there's plenty of nuance i'm overlooking, such as perf implications 😅.

@leeoniya leeoniya closed this as completed Apr 3, 2023
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

No branches or pull requests

2 participants