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

fix: insert empty text nodes during hydration, where necessary #9729

Merged
merged 18 commits into from
Jan 31, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 30, 2023

#9722 fixed some stuff but broke other stuff. We shouldn't insert whitespace for empty expressions, as that will cause visible hydration mismatch flickers — we really need to try and find a different solution to whatever issue it was fixing (the PR references #9694 but that seems to be concerned with something else?)

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Nov 30, 2023

🦋 Changeset detected

Latest commit: 1159b70

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris Rich-Harris changed the title Gh 9722 revert #9722, add regression test Nov 30, 2023
@trueadm
Copy link
Contributor

trueadm commented Dec 4, 2023

We can add a non-visible space instead? The issue is that without anything, there is no actual text node for there to be a sibling.

@Rich-Harris
Copy link
Member Author

I tried non-visible spaces when we were trying to solve the related issue a while back, that doesn't work either. In the other case we fixed it by appending an empty text node when necessary — can we do that here?

@trueadm
Copy link
Contributor

trueadm commented Dec 4, 2023

The PR was meant to fix #9695 btw. I tagged the wrong PR by accident.

@trueadm
Copy link
Contributor

trueadm commented Dec 4, 2023

I tried non-visible spaces when we were trying to solve the related issue a while back, that doesn't work either. In the other case we fixed it by appending an empty text node when necessary — can we do that here?

We can add a comment so we enforce a node being there, but then we'd have to somehow update it to a text node. However, that seems far more expensive and complicated to do. The alternative is that we don't render the space in SSR and on the client we have another operation call $.sibling_frag_text and $.child_frag_text that attempt to insert a text node if one does not exist?

@Rich-Harris
Copy link
Member Author

In the course of merging main, it was necessary to revert #10081. The test in that PR continues to pass. This supports my view that #9722 was an error

@Rich-Harris Rich-Harris changed the title revert #9722, add regression test fix: insert empty text nodes during hydration, where necessary Jan 31, 2024
</script>

{undefined}<hr/>
<button onclick={send}>Send Event</button>
{undefined}<button onclick={() => a += 1}>{a}</button>{undefined}<button onclick={() => b += 1}>{b}</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

because a) the indirectness (pushing to an array in a separate module) is weird, and hard to debug in a REPL compared to watching something change, and b) it only covers the case where the {undefined} is at the start of a fragment, rather than in the middle as well

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.

None yet

2 participants