sveltekit nodes are clobbering each other in "embedded" mode

[john-michael-murphy]

Describe the bug

Consider the scenario where Sveltekit is building pages in embedded mode.

In this scenario, Sveltekit generates two pages: a.html and b.html.

Both a and b are then embedded into a single page:

<ReactWebsite>
<Header />
<a.html />
<Ad />
<b.html />
<Footer />
</ReactWebsite>

When Sveltekit builds these pages, it inlines the following hydration script (or something similar) into both pages:

<script>
{
	__sveltekit_17vqb19 = {
		env: {},
		base: new URL(".", location).pathname.slice(0, -1),
		element: document.currentScript.parentElement
};

const data = [{...}] // page specific props;

Promise.all([
  import("./_app/immutable/entry/start.d2ffe299.js"),
  import("./_app/immutable/entry/app.ef056721.js")
]).then(([kit, app]) => {
  kit.start(app, __sveltekit_17vqb19.element, {
	  node_ids: [0, 2],
	  data,
	  form: null,
	  error: null,
	  params: {},
	  route: {...}
  });
});
</script>

Both pages define the same global variable (in this example, __sveltekit_17vqb19). This puts a.html and b.html in a race condition to define and mount that variable.

This race condition leads a.html and b.html mounting and blowing each other out of the water.

Reproduction

1. Put sveltekit in embedded mode.
2. Define two pages, a and b.
3. Build both pages.
4. Copy the HTML from both a.html and b.html into a third page c.html.
5. Serve c.html

Logs

No response

System Info

System:
    OS: macOS 13.2.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 12.24 GB / 64.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.14.2 - ~/bin/nvm/versions/node/v18.14.2/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 9.5.0 - ~/bin/nvm/versions/node/v18.14.2/bin/npm
  Browsers:
    Chrome: 111.0.5563.146
    Firefox: 111.0
    Safari: 16.3

Severity

serious, but I can work around it

Additional Information

The NYTs has been so happy with Sveltekit and can't thank you enough for all the great work y'all have done.

[benmccann]

Adding a note to myself to help understand the issue: a.html and b.html are two different prerendered pages from the same application

[Rich-Harris]

Sorry for the slow reply @john-michael-murphy!

The hash in the global variable is derived from config.kit.version.name, which defaults to Date.now().toString(). I now realise that's not a very helpful assumption, since it only allows embeds to coexist if they come from different projects.

Ideally the hash would be based on the version and the URL that's being rendered. @ejmurra's PR (hi Eli! I don't think we've met, but any NYTG member is a friend of mine) is close to this, but uses the route instead of the URL which probably isn't enough (you could have two embeds from the same route, if it used dynamic parameters).

The wrinkle is that I think that approach would break the env and base values, since the modules where they're defined need to know the hash at build time:

kit/packages/kit/src/exports/vite/index.js

Lines 339 to 341 in 04837c0

	const global = is_build
	? `globalThis.__sveltekit_${version_hash}`
	: `globalThis.__sveltekit_dev`;

But! I don't think it's a problem that base and env are being redefined (base would likely break stuff if you actually used it, since the embed path and the page path are unrelated; we should probably force base to be absolute in the embedded case but that's a separate conversation). The only real problem here is that both apps are trying to hydrate __sveltekit_blah.element. All we really need to do is take element out of the object.

I think this would work (in other words, putting everything inside the outer curlies so that we can create a local element variable)?

<script>
{
  __sveltekit_17vqb19 = {
    env: {},
    base: new URL(".", location).pathname.slice(0, -1)
  };

+ const element = document.currentScript.parentElement;

  const data = [{...}] // page specific props;

  Promise.all([
    import("./_app/immutable/entry/start.d2ffe299.js"),
    import("./_app/immutable/entry/app.ef056721.js")
  ]).then(([kit, app]) => {
+    kit.start(app, element, {
      node_ids: [0, 2],
      data,
      form: null,
      error: null,
      params: {},
      route: {...}
    });
  });
}
</script>

If that seems like a good solution then that's an easy fix we can make.

[ejmurra]

Hi @Rich-Harris! I've been a svelte user for a while now (we used Sapper to build our covid pages when I was at the Tampa Bay Times) so I'm really excited to get a chance to make a lil contribution here.

Everything you've said makes sense to me - I didn't consider dynamic parameter routes and the need for it to be known at build. I can take a stab implementing this this weekend. I've also been familiarizing myself with the test suite so I think I can write an integration test for this as part of the options test app.

[john-michael-murphy]

@Rich-Harris, now it's my turn to apologize for a slow reply! This solution seems fine to me as long as we don't need that element defined globally!

[john-michael-murphy]

While we're looking at document.currentScript.parentElement do you know why sveltekit moved away from data-sveltekit-hydrate?