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

Allow document level hydration (#5547). #5743

Closed
wants to merge 1 commit into from

Conversation

jimafisk
Copy link

@jimafisk jimafisk commented Dec 4, 2020

Related to issue #5547

This change allows you to target the top level <html> element for hydration. Without this change, if you try this you would receive an error that you can't run insertBefore on Node.

@@ -5,7 +5,9 @@ export function append(target: Node, node: Node) {
}

export function insert(target: Node, node: Node, anchor?: Node) {
target.insertBefore(node, anchor || null);
if (target != document) {
Copy link
Member

@benmccann benmccann Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silently doing nothing seems like the worst of all possible behaviors to me. I'd either append the new element or just leave things as is. this doesn't seem like a very big problem, so I'm not sure it's worth the extra code and complication. if we did want this then you should probably do !== and write a test, but I'm rather hesitant about this addition

Copy link
Author

@jimafisk jimafisk Dec 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @benmccann, thanks for looking at this. I wasn't sure about the right way to approach it, but I'd be happy to revise. My limited understanding is the insert function creates the instructions to mount the elements into the target (in this case the document). The top level document can only have 1 element/child (e.g <html>), so it throws this error. Assuming we use !== it does silently prevent this, but would there ever be a situation where we would want to insert on the document? Maybe there's a better approach though, can you help me understand what I'd need to do to "append the new element?" Thank you!

@benmccann
Copy link
Member

let's leave the discussion on the issue for now until we decide if this is a feature we want. I'm going to close this PR for the time being until we decide this is something we want to pursue

@benmccann benmccann closed this Dec 6, 2020
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

Successfully merging this pull request may close these issues.

2 participants