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

use document.currentScript instead of target option #2221

Closed
Rich-Harris opened this issue Aug 16, 2021 · 14 comments
Closed

use document.currentScript instead of target option #2221

Rich-Harris opened this issue Aug 16, 2021 · 14 comments

Comments

@Rich-Harris
Copy link
Member

Describe the problem

This is a super niche problem to have, but it's one I've faced on two recent projects. Currently, SvelteKit knows which bit of markup to hydrate because of tight coupling between the config...

... and the app template:
<div id="svelte">%svelte.body%</div>

The tight coupling is unfortunate in any case, but where this gets really tricky is if you're using SvelteKit to generate markup that will be embedded multiple times on a page, as in this pseudo-code:

// src/app.html wraps the %svelte.head% and %svelte.body% with these tags
const head_start = '<!-- START:HEAD -->';
const head_end = '<!-- END:HEAD -->';
const body_start = '<!-- START:BODY -->';
const body_end = '<!-- END:BODY -->';

// after building app with adapter-static
for (const file of glob('build/**/index.html')) {
  const html = fs.readFileSync(file);

  const head = contents.slice(contents.indexOf(head_start) + head_start.length, contents.indexOf(head_end));
  const body = contents.slice(contents.indexOf(body_start) + body_start.length, contents.indexOf(body_end));

  const slug = file.slice('build/'.length, -'/index.html'.length);
  const metadata = get_metadata(slug);

  await cms.upload({ ...metadata, head, body });
}

The script ends up hydrating the wrong markup unless you use a placeholder selector instead of #svelte and rewrite it to be unique:

-target: '#svelte',
+target: '#PLACEHOLDER',
-<div id="svelte">%svelte.body%</div> 
+<div id="PLACEHOLDER">%svelte.body%</div> 
-const html = fs.readFileSync(file);
+const html = fs.readFileSync(file).replace(/PLACEHOLDER/g, `svelte-${Math.round(Math.random() * 1e9)}`);

That's pretty ugly (and brittle, since there's a non-zero chance of collisions. We're currently using page slugs instead of random numbers, which avoids collisions, but it turns out that sometimes the same embed will be used multiple times in the NYT live blog...), and involves deep knowledge about how SvelteKit works.

Describe the proposed solution

What if <div>%svelte.body%</div> turned into this?

<div>
  <script type="module">
    import { start } from ".../start-81fdb15d.js";
    start({
      target: document.currentScript.parentNode,
      paths: {...},
      // ...
    });
  </script>

  <!-- the server-rendered markup (if using SSR) -->
</div>

Reasons to do this:

  • we're currently polluting the template with an id="svelte" or similar (which is only used for hydration, but you wouldn't necessarily understand that — it looks like something important that you shouldn't muck about with)
  • tight coupling is bad
  • getting rid of config options is good
  • the markup SvelteKit generates is less portable than it should be

Reasons not to do this:

  • the <script> itself will get hydrated away. maybe that's confusing?
  • IE11 doesn't support document.currentScript, so if we intend to support that browser eventually, this would stand in the way. we could continue to support the target option (if unspecified, defaults to document.currentScript.parentNode), but that would be annoying
  • the browser will encounter the import marginally later. in browsers that don't implement modulepreload that could result in a microscopic startup penalty

Alternatives considered

Alternative is to continue using the (perfectly viable, if finicky and annoying) userland solutions.

Importance

nice to have

Additional Information

No response

@Conduitry
Copy link
Member

Sort of related: #2035. What's suggested here does sound like a nicer solution than that, though. I'm not sure what to do about the IE11 support thing though. It looks like there's a polyfill that works in versions of IE up to 10, but not 11.

@Rich-Harris
Copy link
Member Author

I suppose we could always do this sort of thing:

<div>
  <script type="module" data-hydrate="sveltekit-xyz123">
    import { start } from ".../start-81fdb15d.js";

    const script = document.querySelector('[data-hydrate="sveltekit-xyz123"]');
    script.removeAttribute('data-hydrate');

    start({
      target: script.parentNode,
      paths: {...},
      // ...
    });
  </script>

  <!-- the server-rendered markup (if using SSR) -->
</div>

That way it'll work fine in IE11. The hash could be based on request.path. It's all a bit ceremonial though, and the removeAttribute stuff is only necessary in the rare case you're embedding the same markup multiple times on a page.

@Conduitry
Copy link
Member

Conduitry commented Aug 16, 2021

Actually, I don't think this will work at all, unfortunately. It doesn't look like document.currentScript is set for type='module'. See whatwg/html#997 / https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model / https://developer.mozilla.org/en-US/docs/Web/API/Document/currentScript

MDN suggests using import.meta instead for modules, but that's not helpful because it only gives us access to import.meta.url and not the element itself.

From a quick test, I can confirm that document.currentScript does indeed appear to be null inside a <script type='module'>.

@Conduitry
Copy link
Member

Conduitry commented Aug 16, 2021

I suppose one way of making this work is to convert the inline script into a regular (non-module) one, and use import(...).then(...) instead to get the start function. I'm not sure how that would eventually interact with a feature that lets people inject extra behavior into the client-side JS - would they be prevented from using regular imports unless we transpiled them in some way? Do we want to not worry about that until we get there?

@Rich-Harris
Copy link
Member Author

What about the data-hydrate solution above?

Re injecting extra behaviour, I'm envisaging that if we did that it would look something like this, after bikeshedding:

// src/hooks/client.js

export function beforeStart() {
  // do stuff immediately before init
}

export function afterStart() {
  // do stuff after initial hydration
}

In other words the framework-injected code would call this user-supplied code, and the user wouldn't have to worry about whatever the framework-injected <script> might be doing

@Conduitry
Copy link
Member

Oh, right, the data-hydrate solution wouldn't involve document.currentScript at all. I think the fact that (1) that would work in IE11 and that (2) it avoids needing to switch away from type='module' make that a more appealing option.

Does having a hash based on the URL help solve the particular problem you're facing? It seems like that would be handled by having each script responsible for finding the first thing matching script[whatever-we-call-it] and then removing the attribute, leaving the next script to be able to find itself later on. Unless there's some other reason to have a hash included in the attribute value, I'd say just skip that for now.

@Rich-Harris
Copy link
Member Author

My concern is basically this:

<div class="article-with-embeds">
  <h2>Here is an embed:</h2>

  <div>
    <script type="module" class="secret-sveltekit-attribute">
      import start from 'https://cdn.site.com/first-app/start-xyz123.js';

      const script = document.querySelector('.secret-sveltekit-attribute');
      script.classList.remove('.secret-sveltekit-attribute');

      start({
        target: script.parentNode,
        // ...
      });
    </script>

    <p>embed one markup</p>
  </div>

  <h2>Here is an embed from a different app, or a different version of the same app:</h2>
  <p>It's possible that second-app/start-acd456.js would load before first-app/start-xyz123.js,
       in which case the markup for the first embed would be hydrated by the second app</p>

  <div>
    <script type="module" class="secret-sveltekit-attribute">
      import start from 'https://cdn.site.com/second-app/start-acd456.js';

      const script = document.querySelector('.secret-sveltekit-attribute');
      script.classList.remove('.secret-sveltekit-attribute');

      start({
        target: script.parentNode,
        // ...
      });
    </script>

    <p>embed two markup</p>
  </div>
</div>

@Rich-Harris
Copy link
Member Author

Though I suppose you could get collisions if you had an embed from /foo of app 1 and another from /foo of app 2. So actually the hash would need to be based on something else, or completely random (which feels undesirable)

@Conduitry
Copy link
Member

A hash of all of the HTML that's due to get replaced by hydration maybe? (Will we be able to know that by this point? I think so?) I agree that having it be actually random seems undesirable.

@rmunn
Copy link
Contributor

rmunn commented Aug 17, 2021

As long as the HTML isn't loading any async or defer scripts, it should be possible to get access to the current script with the document.scripts[document.scripts.length - 1] trick, at which point something like the following would work (quick example whipped up in 5 minutes):

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>Document</title>
</head>
<body>
    <div id="one">
        <script>
            console.log(`Script ${document.scripts.length} running now in parent`, document.scripts[document.scripts.length - 1].parentNode);
        </script>
    </div>
    <div id="two">
        <script>
            console.log(`Script ${document.scripts.length} running now in parent`, document.scripts[document.scripts.length - 1].parentNode);
        </script>
    </div>
    <div id="three">
        <script>
            console.log(`Script ${document.scripts.length} running now in parent`, document.scripts[document.scripts.length - 1].parentNode);
        </script>
    </div>
</body>
</html>

I've only tested this in Firefox, but according to https://developer.mozilla.org/en-US/docs/Web/API/Document/scripts the document.scripts property has been around since IE 4, so I'd think this might work.

UPDATE: Sadly, this fails when <script> is changed to <script type="module">. With non-module scripts this produces the expected results, but with module scripts I get "Script 3 running now in parent div id="three"" three times in a row in the console log. So document.scripts is only loaded piecemeal with non-module scripts, but it's fully populated before any module scripts are run. There goes the dead-simple solution that would have worked in IE 11.

@Mlocik97
Copy link
Contributor

Mlocik97 commented Aug 18, 2021

I have one idea, of not totally solving hydratation, but more about app.html and how it should look in project folder.

Let say we will ditch whole app.html (html template), and instead, let user define, what he/she/they needs in config or __layout.svelte (we already have <svelte:head>, <svelte:body>, etc (yes <svelte:lang> is still missing)), and let index.html (template html) be generated by kit instead. Then kit would be able to modify this file as needed, to make sure, all works fine.

@Mlocik97

This comment has been minimized.

@benmccann
Copy link
Member

I'd have a hard time wrapping my head around what it means to have the script hydrate itself. I'm guessing nothing if all goes smoothly, but it seems like it introduces some possibility for making bugs hard to track down when we do have another hydration bug

It seems to me that a lot of the ugliness is in having to update a file on the file system each time you'd like to use a different id. What if there was a getTemplate hook? Then you could update the template dynamically without having to write a new file out to disk. And I think you could just use a simple counter then to make the id unique

@Conduitry
Copy link
Member

Closed via #3674.

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

5 participants