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

feat: implement <noscript> wrapper for lazyloaded images #240

Closed
wants to merge 9 commits into from

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Apr 28, 2021

supporting native lazy loading

Ideally we'd like to support native lazy loading rather than instantiate an IntersectionObserver needlessly.

There are generally speaking two approaches:

  1. render an <img> with a data attribute. When the page starts loading (but before hydration), src can be swapped out with this data attribute if the browser supports lazy loading - but this means that non-JS web scrapers won't see the image, nor will users who have JS turned off in the browser. (Sample library implementing this approach: https://github.com/verlok/vanilla-lazyload).
  2. render <noscript><img></noscript>. When the page starts loading (but before hydration), we swap this out on load for <img> (with src if native lazyloading is supported and an empty gif if not). This ensures that search engines and users without JS can see the image. (Sample library implementing this approach: https://github.com/mfranzke/loading-attribute-polyfill)

A third approach:

  • render an <img> with full src attribute. When the page starts loading (but before hydration), we polyfill lazy loading via an injected script. However, if we load the script post-body, browsers will already have begun requesting the URIs for these images. If pre-body, we can't interact with the DOM to replace or modify these images.

Approach

This PR adopts the second approach above as it solves the SEO question and allows for users to view images who aren't using JS.

It's complicated to render <noscript> with Vue given issues of inherited attributes (for example) so it needs to be injected with a render hook. We also inject a 598 byte minified script into the page body. On page load, but before hydration, this script swaps out the <noscript> for its <img> contents (replacing the src with an empty gif if the browser doesn't support lazy loading). If the browser doesn't support lazy loading, then this script adds a 1.13kB lazy loading polyfill.

TODO:

  • solution that doesn't require @nuxt/image to be added to modules for server-side rendering

Resources:

closes #50, closes #190, closes #202, closes #213

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2021

Codecov Report

Merging #240 (6832b2e) into main (c11946f) will decrease coverage by 0.58%.
The diff coverage is 59.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
- Coverage   62.25%   61.66%   -0.59%     
==========================================
  Files          23       25       +2     
  Lines         559      587      +28     
  Branches      162      169       +7     
==========================================
+ Hits          348      362      +14     
- Misses        211      222      +11     
- Partials        0        3       +3     
Impacted Files Coverage Δ
src/lazy-script.js 0.00% <0.00%> (ø)
src/lazy.ts 100.00% <100.00%> (ø)
src/module.ts 93.33% <100.00%> (+0.22%) ⬆️
src/runtime/components/lazy.mixin.ts 100.00% <100.00%> (ø)
src/runtime/components/nuxt-img.vue 78.78% <100.00%> (-1.86%) ⬇️

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 c11946f...6832b2e. Read the comment docs.

@danielroe danielroe requested a review from pi0 April 28, 2021 12:24
src/lazy-script.js Outdated Show resolved Hide resolved
siroc.config.ts Outdated Show resolved Hide resolved
@pi0
Copy link
Member

pi0 commented Apr 28, 2021

The current approach is depending on dom modification in a pre-body script so why we can't take 3rd option?

@pi0 pi0 marked this pull request as draft April 28, 2021 13:23
@pi0
Copy link
Member

pi0 commented Apr 28, 2021

@danielroe As a suggestion to phase out complexities towards using native, I would suggest first drop the current polyfill system for loading=lazy then we can clearly make a workaround for safari that does not add the overhead of the majority of chrome/ff/modern users.

@danielroe
Copy link
Member Author

danielroe commented Apr 28, 2021

@pi0 The current approach injects the script immediately before the closing body tag, so the DOM has already been parsed. If not wrapped in <noscript> the browser begins requests to image URIs as soon as it encounters them.

I'll think about extracting the polyfill system - challenge will be interacting well with Vue...

@pi0
Copy link
Member

pi0 commented Apr 29, 2021

(PR pending for removing polyfill support first then we can properly benchmark for safari polyfill addition cost and method)

rollup.config.js Outdated Show resolved Hide resolved
src/runtime/scripts/lazy-noscript.js Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@
"vetur"
],
"scripts": {
"build": "siroc build && mkdist --src src/runtime --dist dist/runtime -d --ext js",
"build": "siroc build && mkdist --src src/runtime --dist dist/runtime -d --ext js && rollup -c",
Copy link
Member

Choose a reason for hiding this comment

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

Shall we use unbuild? :))

Copy link
Member Author

Choose a reason for hiding this comment

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

Its time has come! 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

(I'm thinking using unbuild should be a separate PR though.)

@pi0
Copy link
Member

pi0 commented May 12, 2021

Closing in favor of #256 and adding back compat legacy support via placeholder component. (#189)

@pi0 pi0 closed this May 12, 2021
@pi0 pi0 deleted the feat/noscript-lazy-load branch February 17, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants