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

Can't replace top-level HTML element when hydrating #5547

Closed
jimafisk opened this issue Oct 20, 2020 · 10 comments
Closed

Can't replace top-level HTML element when hydrating #5547

jimafisk opened this issue Oct 20, 2020 · 10 comments

Comments

@jimafisk
Copy link

jimafisk commented Oct 20, 2020

Describe the bug
If you try to replace the <html> wrapper during hydration, it throws errors on line 179 of svelte/internal/index.mjs because it can't insertBefore the top-level document. I'm trying to use an entrypoint in this format:

new App({                
  target: document,
  hydrate: true
});

Logs

  • Chrome: DOMException: Failed to execute 'insertBefore' on 'Node': Only one element on document allowed.
  • Firefox: DOMException: Node.insertBefore: Cannot have more than one Element child of a Document

To Reproduce
Unfortunately I don't think I can use the repl to demonstrate hydration issues. This can be seen in the default starter of https://github.com/plentico/plenti. EDIT: Added a demo that doesn't require plenti: https://github.com/jimafisk/svelte-document-replace

Expected behavior
I'd like to be able to use the top-level document as a target without hitting errors. I'm not sure the exact fix to minimize negative impact on other aspects of svelte would be, but a temp fix on line 178 of node_modules/svelte/internal/index.mjs works for me:

function insert(target, node, anchor) {       
  if (target != document) {
    target.insertBefore(node, anchor || null);
  }
}

Stacktraces
N/A

Information about your Svelte project:

  • Your browser and the version:
    • Chrome Version 85.0.4183.102 (Official Build) (64-bit)
    • Firefox Version 81.0 (64-bit)
  • Your operating system: Ubuntu Linux 18.04 LTS
  • Svelte version: 3.23.2
  • Whether your project uses Webpack or Rollup: N/A, I'm using ESM imports

Severity
I currently have to tweak the Svelte lib to make it work with my project, but I recognize that we have a small userbase and there are higher priority items.

Additional context
The error message is the same as shown here, although that seems to be related to a slightly different issue.

Here's another conversation about replacing top-level elements during hydration: https://stackoverflow.com/questions/61640429/how-to-replace-the-contents-of-a-target-in-svelte-instead-of-appending-to-childr

Thank you!

@jimafisk
Copy link
Author

Here's a quick repo that demonstrates the issue: https://github.com/jimafisk/svelte-document-replace

Screenshot

doc_hydrate_error

@Conduitry
Copy link
Member

I'm not sure what you'd need to mount an app at document for, as opposed to mounting it at document.body. Svelte provides a mechanism for manipulating elements in the head (<svelte:head>) - what else do you need? What would you expect mounting at document do differently from mounting at document.body?

@jimafisk
Copy link
Author

One of our primary goals for the project is to simplify the dev experience for new folks as much as possible, so ideally the users won't even have to think about how it's getting mounted at all. Using <svelte:head> is a hugely useful feature, and I suspect folks who are familiar with Svelte will want to use it in our project, but it's a syntax that's foreign to people coming from a strictly HTML background. Our theory is that it will be easier for junior devs to find and manipulate the metadata in a <head> that is included like every other component in their projects.

Also when mounting document.body if folks want to add attributes to the <html> wrapper for instance, they would need to edit a standard HTML file. Currently in our project, the only files people can edit are Svelte templates (we abstract away things like the entrypoint and routing). I'm worried new devs might not understand why you can't do regular Svelte things in this file. In our current setup the HTML wrapper is just a regular Svelte template people can edit like any other template. It gets SSR'd into static HTML that mounts itself - but the user doesn't necessarily need to think about how it creates HTML fallbacks and hydrates them.

We're hoping to make it so the Svelte templates tell a complete story of the dom, while limiting the number of concepts required to get started. Are there challenges to implementing something like this that I might not thinking of? Thanks for taking a look @Conduitry!

@stephanieluz

This comment has been minimized.

@jimafisk
Copy link
Author

jimafisk commented Dec 4, 2020

Hi @Conduitry, would checking if the target is the document in the insert function be an acceptable change to pull in? I created a PR for that here: #5743.

It would help our project out greatly to be able to hydrate the full dom. If this has consequences that I'm not considering, or if you think a different approach would be better please let me know! Thank you!

@benmccann
Copy link
Member

I think that being able to use <svelte:head> and <head> in one component and only <svelte:head> in others would be confusing to users. I think we generally want to avoid having multiple ways of doing something to avoid the potential for confusion

@jimafisk
Copy link
Author

jimafisk commented Dec 6, 2020

You're right, those essentially do accomplish the same thing. We're generally trying to take a content driven approach in our project wherever possible. That way a developer can set up a general template structure and from there everything is fed from the JSON source managed by a content editor. You'd still be able to set things like titles from the content source using <svelte:head> so that's good.

Is there a similar mechanism for reactive attributes on the <html> wrapper? For instance, if someone wanted to switch the lang attr:

<script>
  import Head from './head.svelte';
  import Nav from './nav.svelte';

  export let content;

  let lang = "en"
  const setFrench = () => {
    lang = "fr";
  }
</script>

<html lang="{lang}">
  <Head title={content.title} />
  <body>
    <Nav />
    <button on:click={setFrench}>Fr</button>
  </body>
</html>

I understand that you can't cater the project to everyone's individual preferences, so if this is a change can't pulled in it's ok to close out this issue. I appreciate you both taking the time to discuss options with me. For posterity my main concerns for our particular project are:

  1. <svelte:head> is a new concept outside of the normal flow of vanilla HTML
  2. New folks who want to add a stylesheet, etc might be confused by not finding a head.svelte template
  3. Managing a separate index.html entry point is an extra layer of complexity outside of the main app
  4. Varying top level templates in a reactive manner could be challenging

Thanks!

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@jimafisk
Copy link
Author

jimafisk commented Jul 3, 2021

It looks like this might have been fixed by @hbirler on 6/22/2021:

function insert(target, node, anchor) {
    if (is_hydrating && !anchor) {
        append(target, node);
    }
    else if (node.parentNode !== target || (anchor && node.nextSibling !== anchor)) {
        target.insertBefore(node, anchor || null);
    }
}

This is amazing, thank you so much! 👏 👏 👏

@jimafisk jimafisk closed this as completed Jul 3, 2021
@hbirler
Copy link
Contributor

hbirler commented Jul 3, 2021

Oh wow. I did not even know that this issue existed, so I cannot guarantee that there is no corner case where such a use-case fails. But I guess simply avoiding re-orderings when not necessary results in no exceptions being thrown in your example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants