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

Hydration Mismatch Due to Incomplete Content Download in Slow Connection #8178

Closed
1 task
howagain opened this issue Aug 21, 2023 · 10 comments · Fixed by #8680
Closed
1 task

Hydration Mismatch Due to Incomplete Content Download in Slow Connection #8178

howagain opened this issue Aug 21, 2023 · 10 comments · Fixed by #8680
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: solid Related to Solid (scope)

Comments

@howagain
Copy link

howagain commented Aug 21, 2023

What version of astro are you using?

2.9.0

Are you using an SSR adapter? If so, which one?

NodeJS SSR adapter (in Docker container)

What package manager are you using?

pnpm

What operating system are you using?

Mac

What browser are you using?

Chrome, Safari

Describe the Bug

The page fails hydrating throwing an error of Uncaught (in promise) TypeError: Cannot read properties of null (reading 'nextSibling') also visible with firstChild or firstSibling depending on where the hydration script out ran the HTML.

The issue occurs when hydration is executed before the content of a large astro island is fully downloaded. This leads to a hydration mismatch as the closing tag is missing. It's particularly prevalent with slow network connections, and our setup where JS is fully cached but HTML is not (with a cache-control: no-cache header). It appears to be related to this race condition issue: #7197 but specific to the way Solid works. The problem is not a regression and is found in Astro v2.9.0.

In our example, when hydration breaks, elements that were not hydrated do not console.log on click.

Picture of the error in the console:
Screenshot 2023-08-22 at 8 31 58 AM

What's the expected result?

The hydration should successfully match the opening and closing tags, without any mismatches or breaks, even in slow network conditions with large astro islands.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-qsvzyu?file=README.md

Instructions

  1. Run npm run build and npm run preview to get the built server
  2. Load the page, go to dev tools, turn on cache by unchecking "disable cache", turn on Fast 3G throttling for network speed
  3. Refresh the page several times, occasionally it will throw the Uncaught (in promise) TypeError: Cannot read properties of null (reading 'nextSibling') error

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 21, 2023
@natemoo-re natemoo-re added pkg: solid Related to Solid (scope) - P4: important Violate documented behavior or significantly impacts performance (priority) labels Aug 22, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 22, 2023
@natemoo-re
Copy link
Member

Thanks for the detailed issue and repro! We'll take a look.

@jonyo
Copy link

jonyo commented Aug 25, 2023

We've been trying to make a work-around until this gets fixed, but so far no luck.

@natemoo-re can you think of any temporary work-arounds that might help?

A few things we've tried:

  • Using client:idle - for some reason even using this, when the assets are cached it still hydrates immediately, producing the bug.
  • Attempt to stop propagation of various events used by Astro - I believe since everything is already available it does not delay hydration so fires immediately instead of being delayed by an event. So stoping those events had no effect.
  • Refactor the larger Island so multiple smaller islands: This helped some but not completely. There is a state that adds a lot of complexity, so in that state it still happens, and we cannot break that area up any more than it is.
  • Effectively disable streaming at the cloud level, where the layer waits for all chunks and sends it all at once compressed - this helped a lot, we thought it was fixed at first, but we found the problem still happens under certain circumstances.

We've been fighting with this for a few days now, any ideas on a work-around would be greatly appreciated!

@natemoo-re
Copy link
Member

Hey @jonyo sorry you're still struggling with this! Our team has been heads-down to get 3.0 ready for release and we haven't had a chance to dig into this.

I can think of a few temporary work-arounds that might help.

  1. Wrap the island in a component that ensures the HTML is all buffered before being sent to the browser. Astro.slots.render will return a string with all of its children, so it should be sent in one chunk. Sounds like this may/may not help based on your prior attempt at adjusting streaming.
---
const content = await Astro.slots.render('default')
---
<Fragment set:html={content} />
  1. Add a custom client directive that waits to hydrate until the DOMContentLoaded event fires. This seems pretty foolproof, but definitely report back if you run into any trouble.

@natemoo-re
Copy link
Member

For my own future reference—I'm guessing the ultimate fix here will be to emit some boundary marker (probably a comment) that flags when the island contents have finished streaming.
We'll need to detect (and remove) the comment before attempting to trigger hydration.

We already have a MutationObserver in astro-island#connectedCallback that we can hook into, but my guess is that it triggers eagerly when the first child is present. The boundary should help us determine when all the children have finished streaming.

@jonyo
Copy link

jonyo commented Aug 25, 2023

The boundary should help us determine when all the children have finished streaming.

Also just to mention, it thought the first child was done when it was not in some cases, it had started but not finished. I believe just due to the nature of how the browser parses the DOM and will auto-complete any unclosed tags (where the close tag was not received yet).

It should be fine if you do go with the marker idea, just mentioning it in case someone tries another solution. It would not be enough to wait for the last child due to the auto-completion from the browser, but adding an end marker after the child and waiting for that would due the trick, no chance of the browser thinking the entire node is there when it is not. (if that makes sense)

@jonyo
Copy link

jonyo commented Aug 25, 2023

Github is being goofy and not letting me edit comments...

I meant to start my reply off with a HUGE thanks for the work-around idea! I'm giving that a try now.

@jonyo
Copy link

jonyo commented Aug 30, 2023

Update: Using the custom client directive did the trick! Thanks again for the help, that should hold us over until a "built in" solution is released.

@epsseniyer
Copy link

#7197 May can help you

@jonyo
Copy link

jonyo commented Sep 12, 2023

#7197 May can help you

Hi, and thanks for the suggestion!

That PR does help some, but basically it does not go far enough. Note that that PR is already in the latest version, but the example in the description is still easily reproducible. For a solid island (maybe others as well) that has a lot of content to hydrate, on a slow device the island will still not be fully loaded before it tries to hydrate. At least it won't be when the static JS is cached in the browser. The first time you come to the page it will work fine, but when you reload (and do not have "disable cache" checked) you will get hydration errors and it will not fully hydrate.

@belluzj
Copy link
Contributor

belluzj commented Sep 19, 2023

We have encountered hydration problems for a couple months, without being able to narrow them down, and only now figured out that it was this issue. I think it's extremely important to fix it properly as the default/out-of-the-box behaviour of Astro, as it creates the worst kind of bug: random (depends on network timing), hard to debug (it can affect any random component, as it's not dependent of the user's code for that component but an actual case of "blame the compiler", which reasonable software developers will look into as a last resort), harder to reproduce locally than in production deployment for some reason, and really bad visual output: when using Vue components in our case, when Vue hydrates the half-downloaded HTML, it fills-in the missing bits at the end (to recover from the hydration mismatch) then the missing bits do arrive as downloaded HTML, resulting in the "bottom half" of the component being fully duplicated on screen. It looks really horrible as a result on the page.

Thanks @natemoo-re for the solution 2. Add a custom client directive, that seems to have fixed the issue! Here is some code of the custom client directive in question for the next readers who find this issue:

/**
 * Hydrate after the DOMContentLoaded event
 * See https://github.com/withastro/astro/issues/8178
 * @type {import('astro').ClientDirective}
 */
export default (load, opts, el) => {
  const todo = async () => {
    const hydrate = await load();
    await hydrate();
  };
  // https://stackoverflow.com/questions/39993676/code-inside-domcontentloaded-event-not-working
  if (document.readyState != "loading") todo();
  else window.addEventListener("DOMContentLoaded", todo);
};

This should be the default Astro behaviour, ideally per-island with the idea of having an "end marker" for each island, but I would argue that in the meantime it would be worth fixing the behaviour of all directives to wait for DOMContentLoaded, if that's the only solution that's correct 100% of the time, to save other people from random hydration bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants