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

Getting rid of the svelte-retag wrapper #20

Open
Vanillabacke opened this issue Oct 17, 2023 · 8 comments
Open

Getting rid of the svelte-retag wrapper #20

Vanillabacke opened this issue Oct 17, 2023 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@Vanillabacke
Copy link

Vanillabacke commented Oct 17, 2023

Describe the problem

Using the customElement as a wrapper as itself instead of the ....
I saw your comment:

// TODO: Better than <div>, but: Is a wrapper entirely necessary? Why not just set this._root = this.shadowRoot?

So I guess you should know, what I mean:

svelteRetag({
	component: Comp,
	tagname: 'custom-text',
	shadow: false,
	hydratable: false,
});

and a comp.svelte:

<div>
    <slot></slot>
</div>

it will render as:

<custom-text>
	<svelte-retag> <!-- getting rid of this wrapper and add the functionality on the customt-text-->
		<div>some content of the slot</div>
	</svelte-retag>
</custom-text>

Describe the proposed solution

Havn't tested it fully, but for me it is working with shadow: false:

/**
 * Setup a wrapper in the light DOM which can keep the rendered Svelte component separate from the default Slot
 * content, which is potentially being actively appended (at least while the browser parses during loading).
 */
_initLightRoot() {
  // Recycle the existing light DOM root, if already present.
  let existingRoot = this.querySelector('slot'); // using slot instead of svelte-tag
  if (existingRoot !== null && existingRoot.parentElement === this) {
    this._debug('_initLightRoot(): Restore from existing light DOM root');
    this._root = existingRoot;
  } else {
    // Setup new (first time).
    this._root = document.createElement('slot'); // using slot instead of svelte-tag
    this.prepend(this._root);
  }
}

and in the _renderSvelteComponent() on line 463:

// Instantiate component into our root now, which is either the "light DOM" (i.e. directly under this element) or
// in the shadow DOM.
this.componentInstance = new opts.component({ target: this, props: props, context }); // using target: this instead of this._root

Alternatives considered

would be nice to reduce the the nested tags, but it's just a nice to have. tried to figure out the tests, but I'm not using them yet and got a lot of expection / received errors of the changed render output.

Importance

nice to have

Note from @patricknelson

Address outstanding TODO: ISSUE-20 items.

@Vanillabacke Vanillabacke added the enhancement New feature or request label Oct 17, 2023
@patricknelson
Copy link
Owner

patricknelson commented Oct 18, 2023

FWIW: Coincidentally, that particular code comment was in reference to the wrapping of the shadow DOM, which was added here and was a bug fix, since I was addressing layout issues induced by the <div> wrapper that was there previously (itself a legacy of the upstream repo, see here). But yeah, ideally we wouldn't need it there at all for the shadow DOM either.

That said, I do agree that the use of <svelte-retag> as a wrapper is aesthetically inelegant. It does leave a bit of a "residue" in the final rendered light and shadow DOMs. It's a nagging issue in the back of my mind and it's something that has been brought up by others in a few other instances (e.g. here and here). On that first one, I did take a shot at a quick explanation on that issue last week. However, it's complicated and I don't think I've fully summarized it anywhere, so writing it down here will be great for reference. Especially my own!

Unfortunately, as I started to write it out, it started to turn into a pretty massive thesis, so I'll just give short bullet points. I'll save what I wrote for a post later when I hopefully have time to fill it out a bit more.

Why the <svelte-retag> wrapper currently exists

Note that these only help to explain how we got here, not necessarily a justification for it to continue to exist per se.

There may be a few more use cases, but as you noticed in your attempt above, it's not 100% necessary in all scenarios, particularly once rendering in the light DOM has completed (and of course assuming slots don't change, which of course svelte-retag doesn't handle yet anyway).

More to come later.

Edit: Later has come. See: #20 (comment)

@patricknelson patricknelson added the help wanted Extra attention is needed label Oct 18, 2023
@patricknelson
Copy link
Owner

Note: Just adding help wanted in case anyone wants to take a stab at removing the <svelte-retag> wrapper in final rendered DOM content (at least in the light DOM). We just need to ensure we pass unit tests (which run automatically if a PR is issued).

Just note that some of the older unit test code (Simple.iife.test.js or TestAttributes.test.js) perform basic HTML string comparisons, so they'll break if <svelte-retag> is missing. So in those situations, you would instead wanna use query selectors to validate results (like in newer tests like Context.test.js).

Thankfully unit testing is easy to setup, just clone down and set it up:

npm i
npm run vitest

@Vanillabacke
Copy link
Author

Thank you for sharing your thoughts and background information!
Treat it as an added benefit, and there's no need to lose sleep over it.
And as long as you are not a developer, you wouldn't see it 😅

I'd like to contribute more, but I have more of a 'rudimentary developer' skill set.

@patricknelson
Copy link
Owner

No worries at all. You’re not the only one to notice and ask about it. It appears to not affect the performance at all, but it is an interesting puzzle (getting the end result we want while still having all the features we need).

@patricknelson
Copy link
Owner

Lumping this into v2 since technically this could be a breaking change if anyone has unit tests (or anything else) that somehow depends on the presence of the <svelte-retag> tag being present in the DOM or resulting HTML. Note that it could be bumped in case other features that I want to release earlier are all completed before this one (since help is wanted here).

@patricknelson patricknelson added this to the v2 milestone Dec 4, 2023
@patricknelson
Copy link
Owner

Hmm... sorry folks, still figuring this out! 😅

I need some way to classify something as a breaking change (and thus must be in a future version) but also want a clear way of communicating what will definitely make it into the next version (i.e. sort of like approved for v2 and v3, etc). So for now it cannot be v1 but I'm not sure it'll make it into v2 either, so I'll create a new "one day" milestone instead.

@patricknelson patricknelson modified the milestones: v2, one day Dec 4, 2023
@patricknelson
Copy link
Owner

patricknelson commented Dec 9, 2023

@Vanillabacke it's been a minute since I've gotten back on this with any real detail. That said, I've taken some time today to really dive into this myself and I can provide a bit more feedback. At least on the biggest use case for the <svelte-tag> wrapper, which is my main use case right now.

Essentially, that wrapper is central to solving several complicated problems all together in combination:

  1. Light DOM rendering
  2. iife/umd compilation
  3. Slots (in the light DOM)
  4. CLS (Cumulative Layout Shift)
  5. Speed ⚡ (we make sure you see it ASAP) and performance 🚀 (it takes as little CPU as possible) -- more on both of these below.

Technical breakdown

Each has some kind of tradeoff that factors into the other in some form. The crux of the issue boils down to how browsers perform DOM rendering. On initial page load, the browser will piece together the DOM from the top down. During this phase (i.e. document.readyState is loading), HTML elements are already starting to show on the page. So, if you want your web component to be ready/visible ASAP and not succumb to flicker, layout shift or any sort of "jank" if you well: This is when you need to begin rendering. Also: There is a grace period offered by requestAnimationFrame, e.g. say you're running at 60fps you might have ~17ms or so before the next visual feedback. During this in between time, the browser is getting ready to paint a new frame to your monitor, so svelte-retag is monitoring for updates to slots and is queueing up to render (but only once per frame).

However: During this loading phase, there's simply no way to actually know when all the children in the component's slot are fully available. For example, if you have <wrapper-tag> which has 10 or 100 <p> tags inside of it, there's literally no way to know once you've reached </wrapper-tag>. There's no event that gets fired. There's no hook or callback. We just have to guess or simply wait until document.readyState reaches interactive or completed, which can be ages in the world of performance, especially if you have a large page with a lot of components or a lot of resources to load.

So: Since we don't want to miss too many frames and create any "jank" as the page is loading, we try to render the component somewhere. The <svelte-retag> wrapper in this case works as a target which, importantly, is at the top of the element and out of the way of the parser as it walks along the DOM. Ultimately, we want to ensure that your Svelte component has rendered the instant that slot contents are completed, so we use a temporary MutationObserver to immediately queue (or re-queue) into requestAnimationFrame so that it will trigger a re-render of the component before the next frame paints to screen.

Speed ⚡

The best speed is with iife/umd compilation methods where components (at least the most important ones) are loaded in the <head>. This means all the code for the component is loaded up front and as the parser walks through the DOM, it's able to render your Svelte component like it's a built-in HTML element (i.e. immediately).

Performance 🚀

Lots of things could potentially trigger a re-render (e.g. DOM appending new child nodes directly under your tag), so requestAnimationFrame is used to queue it up to ensure that it only runs once per frame. This is also an important factor when accounting for context, where we compensate for the random/stochastic nature of connectedCallback() initializaiton of custom elements, since Svelte context requires rendering/initialization from the top down. So, to accomplish this, we ensure we try to render only once (if possible) from the top-down in "document order" (i.e. document.querySelectorAll()) in order to retain context.

Overlap ⚡ + 🚀

There's some overlap of course; MutationObserver is relatively efficient and also instantly notifies us when changes occur. But to ensure we don't cascade a ton of updates all at once, we queue them up with requestAnimationFrame so they only run when it's useful and also when we've had some extra time to accumulate more of (or all of) the slot contents. We also double check a few times to ensure we're not wasting resources by rendering a component lower down when we already know one higher up is pending an update.

Alternatives considered

I've been able to accomplish all of the above without the <svelte-retag> wrapper. However, it came with trade offs, e.g.:

  • Tried html-parsed-element, however that results in high CPU usage by traversing the DOM copiously, checking for nextSibling on nodes elsewhere in the DOM. All just to infer that the current tag has completed parsing. Just on the demo site's IIFE page, profiled it taking ~500ms or more (iterating hundreds of times). This does not scale well.
  • I've tinkered a little with some post loaded (i.e. DOMContentLoaded a.k.a. document.readyState is completed) "unwrapping" of the <svelte-retag> tag itself to tidy things up a bit. However, this comes with another tradeoff of creating an unnecessary stall/lockup as the browser goes through all elements. You're potentially locating a ton of HTML elements inside of the wrapper.
  • Don't use iife/umd: Use module to load scripts. This is totally fine, it just comes with CLS and jank since (architecturally), module scripts are always deferred and will not execute until interactive.
  • Hydration offers some potential for a way out of this mess (see Enable hydration of pre-rendered Svelte custom elements #17). Basically, you can workaround the issues of CLS and <svelte-retag> wrappers by having the HTML pre-rendered, the CSS already ready to go and then just "hydrating" with JavaScript interactivity (even re-rendering one last time client-side) once the module has loaded. This isn't really possible yet. The main use case for svelte-retag is for folks like me who have a non-JavaScript back-end. I've thought about maybe a reverse HTTP proxy (or even a service worker) running node + jsdom + svelte-retag to do quick "man in the middle" style rendering of components from PHP/serverside generated HTML (sitting in between the user somehow and the back-end server). But... that's a can of worms! 🥵

Conclusion

To me, at least for light DOM (using iife/umd) the tradeoffs for the options I've explored so far simply aren't worth it when the downside is just that it's a bit harder to read, particularly for performance. That said, I think it's feasible to probably undo <svelte-retag> for normal module style loading, so this issue should stay open while I work on that. 😅

I'll also continue to do more research on ways to balance the best of all points noted above with iife/umd as well, but it may not be practical, at least not for now.

More reading

This is a common frustration. See:

@patricknelson
Copy link
Owner

Note: One important gradual step in this direction is tweak the older unit tests that depend on the presence of the <svelte-retag> wrapper, since they're using HTML instead of DOM queries. WIP here: #46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants