Skip to content
This repository has been archived by the owner on Oct 13, 2022. It is now read-only.

Fix duplicated elements on hydration #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix duplicated elements on hydration #306

wants to merge 1 commit into from

Conversation

babeard
Copy link

@babeard babeard commented Mar 17, 2021 •
edited
Loading

Fixes a problem where elements were being duplicated upon re-hydration on slower connections. See sveltejs/sapper#1725

@babeard babeard changed the title Fix duplicate elements (sveltejs/sapper#1725) Fix duplicated elements on hydration Mar 17, 2021
@benmccann
Copy link
Member

thanks for tracking this down. I wonder if the better fix wouldn't be to wait to call start on load instead instead in https://github.com/sveltejs/sapper-template/blob/master/src/client.js

benmccann
benmccann reviewed Mar 17, 2021
View reviewed changes
src/template.html Outdated Show resolved Hide resolved
benmccann
benmccann reviewed Mar 17, 2021
View reviewed changes
src/template.html Outdated Show resolved Hide resolved
@Conduitry
Copy link
Member

Huh. What exactly needs to wait for what - and what needs to be fixed - now that Sapper is using <script defer>? sveltejs/sapper#1123

@babeard
Copy link
Author

babeard commented Mar 17, 2021

Defer or not doesn't seem to affect the hydration/duplication problem unfortunately. I'm not sure why because spec says that deferred scripts are fetched in parallel and evaluated when the page has finished parsing. I'm not quite sure what else would be going on beside's Chrome not strictly adhering to spec. In Firefox, for example, this issue does not appear.

Why is the window load event preferred over moving scripts to the end of the body? Just curious.

Also, sorry for the poor "gitfu". I'm learning as I go. Any pointers are appreciated!

@benmccann
Copy link
Member

Why is the window load event preferred over moving scripts to the end of the body? Just curious.

The the script is encountered soon and can start being parsed sooner. Probably doesn't make much of a difference with the small document size, but it is better practice

benmccann
benmccann suggested changes Mar 17, 2021
View reviewed changes
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I'm also questioning now whether the document not being loaded yet would result in the behavior being seen. If you try to create a Svelte app when the target element is present you get the message 'target' is a required option, so it wouldn't end up creating them twice and I don't think it's the case that the script is being run before the target element is present unless I'm missing something

@babeard
Copy link
Author

babeard commented Mar 18, 2021 •
edited
Loading

I'm also questioning now whether the document not being loaded yet would result in the behavior being seen.

My original hypothesis may be totally incorrect, but the two fixes below do resolve the issue we're seeing. Now the question is why. Do you think Chrome's disk cache retrieval could be at play here? As in the cached resource not being deferred but being retrieved and evaluated before it needs to be?

Brief summary of observations

  • Chrome based browsers only
  • Initial SSR renders correctly, then duplicate elements appear (presumably on hydration)
  • Issue appears when on slower network and in combination with a larger client-[hash].js (in this case, it's png files inlined via base64)
  • Initial page load doesn't seem to be affected, only on subsequent refreshes.
  • Disabling cache via chrome devtools eliminates the issue as well.
  • Disabling service-worker does not fix.
  • Occurs in all versions of Sapper down to and including 0.27.16. (I have not tested below that). I believe the reason that quite a few people are seeing this error when migrating from 0.27 is because the migration guide suggests %sapper.scripts% can be moved to the <head> section for slightly better performance.
  • It appears svelte-kit does not inherit this problem 🥳

Working Fixes

  • append %sapper.scripts% to body
  • start sapper client on window load event (currently this PR)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benmccann benmccann benmccann requested changes

@antony antony Awaiting requested review from antony

@Conduitry Conduitry Awaiting requested review from Conduitry

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@babeard @benmccann @Conduitry