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.

Sign up for GitHub

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

Jump to bottom

kit start mounting wrong node #4685

Closed
john-michael-murphy opened this issue Apr 21, 2022 · 4 comments · Fixed by #4972
Closed

kit start mounting wrong node #4685

john-michael-murphy opened this issue Apr 21, 2022 · 4 comments · Fixed by #4972
Milestone
1.0

Comments

@john-michael-murphy
Copy link

john-michael-murphy commented Apr 21, 2022 •
edited
Loading

Describe the bug

For a number of reasons related to platform integration, nytimes wraps sveltekit html output in a template that will occasionally hoist script tags into the body tag:

src/hooks.js

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
	return resolve(event, {
		transformPage: ({ html }) => {
                          // wraps the output in the nytimes renderer, which sometimes 
                          // hoists script tags contained in html out of the generated component html and into the <body> tag.
			return render_page_in_template(html);
		},
	});
}

Since kit now assumes that the mount script is always an immediate child of the root mount node, this means that kit will mount the body node instead of the parentNode defined in app.html template.

In practice this means that if we wanted to embed a component generated by kit into the NYTs homepage (which hoists scripts), we're at risk of that component blowing out the entire DOM.

I really like the ergonomics introduced in #3674, but I wonder if instead setting data-hydrate on <script data-hydrate="foo" /> we set it on a tag that's not vulnerable to hoisting <div data-hydrate="foo" />

Thanks so much for all the amazing work y'all have done so far. And thanks in advance for your wisdom on this.

Reproduction

In src/hooks.js write a small script that hoists kit script tags out of their default parent node.

Logs

No response

System Info

System:
    OS: macOS 12.3.1
    CPU: (10) x64 Apple M1 Max
    Memory: 2.44 GB / 64.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.18.3 - ~/bin/nvm/versions/node/v14.18.3/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 6.14.15 - ~/bin/nvm/versions/node/v14.18.3/bin/npm
  Browsers:
    Chrome: 100.0.4896.127
    Firefox: 99.0
    Safari: 15.4
  npmPackages:
    @sveltejs/adapter-static: 1.0.0-next.29 => 1.0.0-next.29 
    @sveltejs/kit: 1.0.0-next.305 => 1.0.0-next.305 
    @sveltejs/vite-plugin-svelte: 1.0.0-next.41 => 1.0.0-next.41 
    svelte: ^3.44.0 => 3.46.4 
    vite: ^2.6.10 => 2.8.4

Severity

blocking an upgrade

Additional Information

No response
@john-michael-murphy john-michael-murphy changed the title Svletekit mounting wrong node kit start mounting wrong node Apr 21, 2022
Rich-Harris added a commit that referenced this issue Apr 22, 2022
@Rich-Harris
make hydration resilient to hoisted <script> elements (#4685)
df83088
@Rich-Harris Rich-Harris mentioned this issue Apr 22, 2022
Closed
5 tasks
@Rich-Harris
Copy link
Member

I've opened #4698, which solves this, but at the cost of an additional element in all server-rendered HTML. While working on it, it occurred to me that you could probably achieve the same outcome using transformPage, I think?

const pattern = /data-hydrate="(.+)"/;

const response = await resolve(event, {
  transformPage: ({ html }) => html
    .replace(/<script[^]+?<\/script>/g, (script) => {
      const match = pattern.exec(script);
      return match ? `<span ${match[1]}/>${script.replace(pattern, '')}` : script;
    })
});

@john-michael-murphy
Copy link
Author

@Rich-Harris you're so fast! Thank you!

We're already doing the string manipulation you suggest in the comment. The concern with that approach is that it's modifying an internal sveltekit implementation detail that could change without warning. If y'all are aware of this edge case and don't think anything about the current implementation will change in the future, I'm super happy keeping that string replacing as our final solution! Otherwise, #4698 looks great!

@Rich-Harris Rich-Harris added this to the 1.0 milestone Apr 23, 2022
Rich-Harris added a commit that referenced this issue May 17, 2022
@Rich-Harris
add test to prevent accidental breakage of data-hydrate mechanism - c…
2d0d1df 
…loses #4685
@Rich-Harris Rich-Harris mentioned this issue May 17, 2022
Merged
5 tasks
Rich-Harris added a commit that referenced this issue May 18, 2022
@Rich-Harris
add test to prevent accidental breakage of data-hydrate mechanism (#4972
8dd0e36 
)

* add test to prevent accidental breakage of data-hydrate mechanism - closes #4685

* change data-hydrate to data-sveltekit-hydrate

* changeset
@john-michael-murphy
Copy link
Author

Thanks, folks!

@john-michael-murphy
Copy link
Author

@Rich-Harris it looks like this may have regressed. Would you recommend we manage the html script ourselves?

@john-michael-murphy john-michael-murphy mentioned this issue Mar 23, 2023
Closed
@samjacoby samjacoby mentioned this issue Jul 19, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0
Development

Successfully merging a pull request may close this issue.

add test to prevent accidental breakage of data-hydrate mechanism sveltejs/kit
2 participants
@Rich-Harris @john-michael-murphy