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

Prevent image flicker and reloading by checking attribute before setting it #3579

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Prevent image flicker and reloading by checking attribute before setting it #3579

merged 1 commit into from
Oct 17, 2019

Conversation

tcrowe
Copy link
Contributor

@tcrowe tcrowe commented Sep 17, 2019

What does it do?

Before setting an attribute it checks if the value is different.

Why?

If you look at the network log in the developer console you will notice that if you render SSR, then hydrate, it will reset the src attribute, and then re-download the image.

Usually the image is cached so the time to load it is very fast. However it still flickers when doing this.


I think the merits of this approach should be debated but it's useful in my project and the tests still pass.


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

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

I'm not sure how to check for image re-downloads in the tests. Sorry!

Tests

  • Run the tests tests with npm test or yarn test)

@codecov-io
Copy link

codecov-io commented Sep 17, 2019

Codecov Report

Merging #3579 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3579   +/-   ##
=======================================
  Coverage   50.25%   50.25%           
=======================================
  Files           1        1           
  Lines         197      197           
=======================================
  Hits           99       99           
  Misses         98       98

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c5ccf6...552f19b. Read the comment docs.

@tcrowe
Copy link
Contributor Author

tcrowe commented Sep 17, 2019

  1. Loading twice

Screen Shot 2019-09-16 at 8 31 25 PM

  1. The network log points to setAttribute

Screen Shot 2019-09-16 at 8 31 36 PM

tcrowe added a commit to tcrowe/svelte-hydrate-test that referenced this pull request Sep 17, 2019
@tcrowe
Copy link
Contributor Author

tcrowe commented Sep 17, 2019

If you wish to reproduce it I've made this and updated it for this case:
https://github.com/tcrowe/svelte-hydrate-test

@Rich-Harris Rich-Harris merged commit 99911c3 into sveltejs:master Oct 17, 2019
@Rich-Harris
Copy link
Member

Thank you. Man, that's horrendous — I somehow hadn't encountered that behaviour before. Browsers 🙃

A silver lining here is that if we need to check the current attribute value for correctness anyway, we don't need to bother keeping the current value in memory (which happens for complex attribute values) — in other words we can simplify the generated code a bit. I'll address that in a follow-up

Rich-Harris added a commit that referenced this pull request Oct 17, 2019
@tcrowe tcrowe deleted the set-attribute-only-if-different branch October 20, 2019 20:05
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.

3 participants