fix: make hydration more html whitespace tolerant #16243
Conversation
🦋 Changeset detectedLatest commit: 94c4784 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
.envrc
Outdated
There was a problem hiding this comment.
this and the flake files seem like accidental commits - could you remove them?
There was a problem hiding this comment.
Sure. I just did. I also commited my .gitignore changes, as a separate commit. Is that acceptabl? Otherwise I can remove 94c4784 again.
There was a problem hiding this comment.
why did you remove these tests?
There was a problem hiding this comment.
I did not remove these tests, I changed them: They were asserting a hydration failure, but with this fix hydration does not fail anymore.
Well, at least it was not my intention to remove them. What I know for sure is that the respective tests' folders including the test .svelte components still exist and the test count did not decrease. Without this change these tests will fail with the - unfulfilled- expectation of a failed hydration.
There was a problem hiding this comment.
Upon further investigation I realized cloudflare-mirage-borking should still continue to fail hydration, as the failure is unrelated to this fix (additional element inserted, so not a seatically meaningless whitespace change). I updated to PR to only expect successful hydration for cloudflare-mirage-borking-2.
|
one of the cases we CAN hydrate
this fixes sveltejs#16242 and allows to sucessfully hydrate sveltejs#15851 as well.
For some reason await blocks render html whitespace and their custom hydration error handling seems to depend on hydrating text nodes.
| */ | ||
| export function hydrate_next(allow_text = false) { | ||
| var node = set_hydrate_node(/** @type {TemplateNode} */(get_next_sibling(hydrate_node))); | ||
| while (!allow_text && node.nodeType === TEXT_NODE && !node.nodeValue?.trim()) { |
There was a problem hiding this comment.
Trimming "breaks" things - this is why you did hydrate_next(true) for the await block, but similar thing can be in any block. Use
import { test } from '../../test';
export default test({
before_test() {
window.document.body.insertBefore(window.document.createTextNode(''), window.document.querySelector('main'))
}
});to insert empty text node during testing.
There was a problem hiding this comment.
Yes, I realized it when running the full test-suite. However, the await case was the only one in the entire test suite that seem to rely on whitespace within html. So, allowed for the new logic to be disabled for that case. Notably, this isn't even a hydration test.
On a more conceptual level it is a bad idea to rely on whitespace within html to be preserved. There are countless tools - internal and external - that do non-semantic whitespace changes in html.
Unfortunately, I don't know enough about why exactly preserving whitespace is currently needed, to propose a more principled solution. All I can say is that the new test I added should at least fail gracefully and not crash the entire app resulting in a blank page after hydration.
There was a problem hiding this comment.
Also browsers mostly ignore whitespace, so as a developer I didn't expect the observed crash to blank page of a complex production app to be caused by hydration not able to handle modified white space. Depending on it, will certainly lead to a terrible dev experience for many more. This would be very incompatible with the svelte experience, you awesome people got me hooked on (pun very much intended). Further reading: https://developer.mozilla.org/en-US/docs/Web/API/Document_Object_Model/Whitespace
There was a problem hiding this comment.
Unfortunately, the tests are a far from covering all possible cases.
The case is the following nodes:
- hydration mark;
- text node (user content +
" ", so it's guarantied it is a non-empty text); - other nodes.
and when the text node happens to be just a whitespace, you remove it, that cases the shift of nodes order.
There was a problem hiding this comment.
Unfortunately, the tests are a far from covering all possible cases.
Ok, I feared that being the case. That makes it much harder for me to contribute a fix.
The case is the following nodes:
hydration mark;
text node (user content +
" ", so it's guarantied it is a non-empty text);other nodes.
If you could provide tests for the relevant cases, I cloud try to find a more general solution.
If not, I suggest we just prevent the failing call to appendChild and handle most html whitespace manipulation as hydration mismatches. I can change my PR to do that. The unfortunate thing is, that I believe pre svelte 5 the hydration was much more robust in that regard.
and when the text node happens to be just a whitespace, you remove it, that cases the shift of nodes order.
Yes, that was expected.
In the test suite it only resulted in a gracefully handled hydration mismatch for the await tests whilst making more hydration tests actually hydrate. So it seemed like a good compromise. To make await work as well I allowed to opt out of text node skipping.
If I understand correctly now, you're saying that relying on text nodes to carry relevance happens far more than the existing tests suggest. In that case I am asking myself if that information could be transmitted in a more reliable way. Thoughts?
There was a problem hiding this comment.
See the REPL (check console) in my first message: this branch is causes hydration_mismatch, on 5.34.8 - all good.
More advanced case where this PR causes crash.
Oh, I seem to misread the issue - I thought GT inserts empty text nodes.
We'll make a more general hydration fix.
There was a problem hiding this comment.
We'll make a more general hydration fix.
Awesome. Should I change this commit to only contain the crashing test case?
Oh, I seem to misread the issue - I thought GT inserts empty text nodes.
Sorry I don't follow. What does GT stand for here?
There was a problem hiding this comment.
Oh, that makes sense 😅
As empty is ambiguous: GT does insert whitespace text nodes. So in HTML they are semantically empty but literally they are not as they contain newline and space characters, which causes the issue in my case.
There was a problem hiding this comment.
BTW. I tried to reproduce the issue within the REPL. Unfortunately without success. So the test in f7ce3f6a9 is the only way to show the crash.
See:
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint