Remove all hydration wrapper divs via "virtual fragments" #101

natemoo-re · 2021-04-15T19:40:16Z

Changes

This PR is not critical, but is meant to open a discussion. There very well might be problems with this approach that I haven't thought of. Please share your thoughts!

This PR removes wrapper <div> elements from client-side Components. The HTML you write is the exactly the HTML that is generated. Why? Because people will try to style client-side components with :nth-child and > * + * selectors, but we're opaquely injecting a wrapper <div> around each one, breaking their assumptions.

Frameworks require you to pass a parent Element for mounting. Your app's markup will be rendered as its children. Requiring a wrapper <div> seems like a technical limitation, but this PR introduces a way to sidestep it.

By passing what I'm calling a virtual fragment, we can control framework rendering behavior by creating a Proxy which safely overrides a few specific DOM methods. These overridden methods narrow the DOM elements controlled by the framework from all children to only a subset of children.

Testing

Check the output of examples/kitchen-sink—no wrappers!

Tests are passing
Tests updated where necessary

Docs

Docs / READMEs updated
Code comments added where helpful

Note We'll still need to hoist all Astro setup scripts to the <head> or end of <body>, which needs to happen in the compiler. optimize runs before these scripts are generated, so we'll need a new hook.

Co-authored-by: Nate Moore <[email protected]>

drwpow · 2021-04-15T19:53:56Z

src/frontend/render/renderer.ts

-    const astroId = `${Math.floor(Math.random() * 1e16)}`;
-    return { ['data-astro-id']: astroId, root: `document.querySelector('[data-astro-id="${astroId}"]')`, Component: 'Component' };
+  const createContext = (toHash: string, i: number) => {
+    const hash = shorthash.unique(toHash);


👍🏻 Haven’t used shorthash. I’ve been looking for a library that does this, but the ones I found were too weighty. I’d like to use this in other places in Astro too (currently scoped styles uses a custom crypto function)

matthewp · 2021-04-15T20:26:17Z

src/frontend/render/renderer.ts

+        const i = (ids.has(value) ? ids.get(value) : -1) + 1;
+        ids.set(value, i);
+        const innerContext = createContext(value, i);
+        value = injectAstroId(value, innerContext['data-astro-id']);


I assume this happens on every render. I think the cost of that is too high.

* chore: add assets * docs: update readme Co-authored-by: Nate Moore <[email protected]>

matthewp · 2021-04-15T20:28:05Z

src/frontend/dom.ts

+
+  const fragment = {
+    parentNode,
+    firstChild: childNodes.item(0),


Very clever hack! Does feel a bit fragile. I can think of a bunch of APIs that a framework might use that's not included in this list (lastChild for example). We're more likely to run into them when we add frameworks (I bet Lit uses more modern apis like append and prepend for example), but the worst part is that React or Svelte or whoever might make a patch release that uses one of the APIs not on this list and our stuff would break :(

They might not have been called "islands", but there’s a rich history of doing things like this to hydrate without a parent. 😄

https://stackblitz.com/edit/react-jp5grx?file=index.js

This technique is performs better than rendering into a fragment and using the render callback to update the actual DOM.

matthewp · 2021-04-15T20:34:12Z

I really appreciate the hack here! It's clever. But I do think the cost is just a bit too high. I can think of a few alternative ideas that might prevent the problems but those all will have their own caveats as well.

I'm not sure there is a good solution to this problem. I think probably any solution has some downsides. I'd prefer we went with the solution that kept our JavaScript size small and didn't affect server rendering performance. We can document that dynamic components require a wrapper element, and that that means you can't use sibling selectors in just this one instance. It's a niche enough need that I don't think there will be many complaints.

natemoo-re · 2021-04-15T20:57:04Z

Totally fair @matthewp! I agree, it's a niche enough use case that it shouldn't be much of a problem. Definitely not worth introducing performance overhead.

The one thing from this exploration that I would still like to apply in another branch is that I really think data-astro-id should be deterministic. The current behavior will cache-bust every static page each time the site is rebuilt. Does this shorthash approach work for you or do you have an alternative suggestion?

eyelidlessness · 2021-04-15T20:58:45Z

@matthewp I definitely think this is worth pursuing. While on the whole sibling selectors may be niche (I’m not sure how true this is, but I’m willing to accept it for the purposes of this discussion), they’re particularly important for many progressive enhancement approaches—such as using :checked and :target selectors to control/indicate state—which will probably be more important to Astro users than the general web dev population.

Another reason to avoid wrapper divs is that deeply nested HTML can negatively impact users who depend on assistive tools like screen readers, and is penalized by:

Browsers’ reader mode algorithms
Search engines
Lighthouse (not as important but worth mentioning)

It can also be a confusing DX. Generally speaking we don’t expect declarative component libraries to produce markup that wasn’t declared.

All of these factors motivated me to stop using Emotion for styling. And particularly the progressive enhancement issues would make it difficult for me to use Astro.

All of that said, I don’t have much context to understand where the costs are incurred or why, but if you wouldn’t mind elaborating I’d be happy to add another mind to the hive to help find a way to make this happen. I have already proposed a few solutions for issues with Microsite’s approach in natemoo-re/microsite#150.

matthewp · 2021-04-15T21:00:52Z

@natemoo-re Yeah, hashing based on the HTML contents makes sense, and I do like keeping it deterministic. Will prevent cache busting every page on each deploy.

matthewp · 2021-04-15T21:07:13Z

@eyelidlessness I don't think it's common to use a sibling select to select something inside of a component. A component is supposed to be a tool for encapsulation. If you use a sibling selector like @natemoo-re's you are going to select the wrapper div. And for a lot of use cases that's perfectly fine. If you need to select the component's top elements you can just change your selector to select them.

The workarounds are minor. Lots of software has caveats, this is a pretty acceptable one imo.

natemoo-re · 2021-04-15T21:14:14Z

Awesome. I'll spin that deterministic rendering fix out into a separate branch. I'll keep this PR open for a short time to facilitate discussion, but we won't be merging it.

IMO the best possible solutions here (in order) would be:

A browser API similar to createDocumentFragment which is treated like a live element
Official framework support for mounting to only a subset of children

This hack is way down that list. It makes sense not to bake this into Astro.

eyelidlessness · 2021-04-15T21:15:42Z

@matthewp The issue isn’t selecting into a component, you’re right that it’s trivial to do, say, :checked ~ .wrapper .foo. I’ve written zillions of selectors like that. The issue is that you can’t select out of one. So if you’re hydrating something that could use a :checked selector to perform a state change, once that input is wrapped the fallback behavior is broken.

This could potentially be worked around in some cases by structuring the input outside the hydrated component, but now you have two separate but tightly coupled components. The exact opposite of encapsulation.

natemoo-re · 2021-04-15T21:19:13Z

I had an alternative idea that might be worth exploring... We could potentially build a single app per-framework which uses Portal components to project "islands" wherever they are needed.

No idea if it's feasible, just a thought experiment for now.

matthewp · 2021-04-15T22:52:26Z

@eyelidlessness That's a good point. I wonder if this is something we could facilitate with our CSS implementation. For example we could attach a class name to the wrapper element (I would prefer this be a custom element rather than a div but that's another topic). You could use that class name to perform sibling selection.

The tough part is that you would need to know the name of that class and it would have to be distinct to only that element instance. Maybe the astro Id could be used for this some how?

natemoo-re · 2021-04-15T23:03:57Z

Hmm, not suggesting that we overload a real thing, but maybe we could expose something like :host?

eyelidlessness · 2021-04-16T00:43:30Z

@matthewp @natemoo-re Unfortunately that wouldn’t work. Pseudoclass selectors (and any selectors) can’t break out of their containing hierarchy. So you can’t do something like .parent-of(:checked) ~ .foo. Once an element has a parent it has no way of selecting its parents’ siblings.

eyelidlessness · 2021-04-16T00:45:53Z

I blame jQuery for this being hard to discuss by inventing selectors that had a DOM/xpath basis but no basis in standards reality

natemoo-re · 2021-05-03T17:32:53Z

Closing this PR as it won't be merged. If warranted, further discussion can take place in an issue.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

fix(www): link styles (#100)

2f3c111

Co-authored-by: Nate Moore <[email protected]>

drwpow reviewed Apr 15, 2021

View reviewed changes

drwpow approved these changes Apr 15, 2021

View reviewed changes

natemoo-re requested a review from matthewp April 15, 2021 20:17

matthewp reviewed Apr 15, 2021

View reviewed changes

Add assets/ (#102)

3e6071e

* chore: add assets * docs: update readme Co-authored-by: Nate Moore <[email protected]>

matthewp reviewed Apr 15, 2021

View reviewed changes

Nate Moore added 10 commits April 15, 2021 15:28

docs: fix readme

caf3625

docs: fix readme

3deb160

chore: remove github banner

d2337e7

feat: mount components on virtual fragment root

f366081

fix(lint): disable @typescript-eslint/ban-ts-comment

b304eb6

fix: vue behavior

f1a5320

refactor: import render for React

c3f4bcf

chore(examples): use visible for kitchen-sink

7b710bc

fix: add __astro_clear, removeEventListener

f0d2c35

chore: cleanup kitchen-sink

5fe2dbe

natemoo-re force-pushed the fragment-hydration branch from 1ef0ed1 to 5fe2dbe Compare April 15, 2021 20:33

drwpow force-pushed the main branch from d2337e7 to 2a7aa76 Compare April 16, 2021 06:15

natemoo-re closed this May 3, 2021

natemoo-re deleted the fragment-hydration branch August 11, 2021 18:57

ematipico pushed a commit that referenced this pull request Feb 6, 2025

[ci] release (#101)

acb9241

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all hydration wrapper divs via "virtual fragments" #101

Remove all hydration wrapper divs via "virtual fragments" #101

natemoo-re commented Apr 15, 2021

drwpow Apr 15, 2021

matthewp Apr 15, 2021

matthewp Apr 15, 2021

jonathantneal Apr 15, 2021

matthewp commented Apr 15, 2021

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 15, 2021

matthewp commented Apr 15, 2021

matthewp commented Apr 15, 2021 •

edited

Loading

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 15, 2021 •

edited

Loading

natemoo-re commented Apr 15, 2021 •

edited

Loading

matthewp commented Apr 15, 2021

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 16, 2021

eyelidlessness commented Apr 16, 2021

natemoo-re commented May 3, 2021

Remove all hydration wrapper divs via "virtual fragments" #101

Remove all hydration wrapper divs via "virtual fragments" #101

Conversation

natemoo-re commented Apr 15, 2021

Changes

Testing

Docs

drwpow Apr 15, 2021

Choose a reason for hiding this comment

matthewp Apr 15, 2021

Choose a reason for hiding this comment

matthewp Apr 15, 2021

Choose a reason for hiding this comment

jonathantneal Apr 15, 2021

Choose a reason for hiding this comment

matthewp commented Apr 15, 2021

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 15, 2021

matthewp commented Apr 15, 2021

matthewp commented Apr 15, 2021 • edited Loading

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 15, 2021 • edited Loading

natemoo-re commented Apr 15, 2021 • edited Loading

matthewp commented Apr 15, 2021

natemoo-re commented Apr 15, 2021

eyelidlessness commented Apr 16, 2021

eyelidlessness commented Apr 16, 2021

natemoo-re commented May 3, 2021

matthewp commented Apr 15, 2021 •

edited

Loading

eyelidlessness commented Apr 15, 2021 •

edited

Loading

natemoo-re commented Apr 15, 2021 •

edited

Loading