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

Add essential support for WebP/AVIF in @docusaurus/lqip-loader and @docusaurus/plugin-ideal-image #8686

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tats-u
Copy link
Contributor

@tats-u tats-u commented Feb 19, 2023

sharp does support both.
Generated lqips for WebP/AVIF have the same formats as inputs, but different formats may be better. (needs improvement if so)

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

sharp used in @docusaurus/lqip-loader does support WebP/AVIF, but @docusaurus/lqip-loader claims that it does not support them. This prevents plugin-ideal-image from supporting WebP/AVIF.
This PR is to add minimal support (may be improved later) for them.

Test Plan

  • Check magic numbers (included in unit tests)
  • (How should we preview using browsers?)

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

(Should I create one?)

Note

PNG & WebP & AVIF are rasterized using Inkscape (96dpi; 200x200).
All of them are optimized by Squoosh.

  • PNG: OxiPNG
  • WebP: lossless
  • AVIF: YUV444 & quality 30

AVIF's lqip may be so large that we have to use WebP instead.

Blocked by libvips / sharp

  • Can tell lossless WebP from lossy
  • Can detect chroma subsampling method (e.g. YUV444 / YUV420) in AVIF like JPEG

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 19, 2023
@netlify
Copy link

netlify bot commented Feb 19, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit d227356
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63f74468f58e1d00083cc09a
😎 Deploy Preview https://deploy-preview-8686--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Feb 19, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 75 🟢 97 🟢 92 🟢 100 🟢 90 Report
/docs/installation 🟠 78 🟢 100 🟢 92 🟢 100 🟢 90 Report

@tats-u
Copy link
Contributor Author

tats-u commented Feb 19, 2023

I CAN'T BELIEVE
image

@tats-u tats-u marked this pull request as ready for review February 19, 2023 15:40
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Could you also add some real tests on our own website in website/_dogfooding?

@tats-u
Copy link
Contributor Author

tats-u commented Feb 23, 2023

@slorber Could you tell me what kind of tests I should add? I have no idea.
This changes doesn't affect Docusaurus itself without changes on other plugins (e.g. plugin-ideal-image).

@slorber
Copy link
Collaborator

slorber commented Feb 23, 2023

We want to add some webp/avif images to some hidden docs in our own website, to make sure that the feature is working and to be able to review it in a deploy preview link

The Docusaurus website includes a large set of hidden test pages that completes the unit tests: https://docusaurus.io/tests

@tats-u
Copy link
Contributor Author

tats-u commented Feb 23, 2023

@slorber No materials are present now and this modification itself doesn't affect on Docusaurus without additional Webpack settings. The plugin-ideal-image modification will be at another PR. (not included in this PR)

$ fd  "\.(webp|avif)$"  
packages\lqip-loader\src\__tests__\__fixtures__\docusaurus.avif
packages\lqip-loader\src\__tests__\__fixtures__\docusaurus.webp

@slorber
Copy link
Collaborator

slorber commented Feb 23, 2023

@slorber No materials are present now and this modification itself doesn't affect on Docusaurus without additional Webpack settings. The plugin-ideal-image modification will be at another PR. (not included in this PR)

$ fd  "\.(webp|avif)$"  
packages\lqip-loader\src\__tests__\__fixtures__\docusaurus.avif
packages\lqip-loader\src\__tests__\__fixtures__\docusaurus.webp

I don't understand what you mean.

I'm asking you to add an image of each type to our website. If they do not exist, then create them, or use the existing fixtures you already added.

I want to see the "minimal support" in action on our own site in the deploy preview here: https://deploy-preview-8686--docusaurus-2.netlify.app/tests/

@tats-u
Copy link
Contributor Author

tats-u commented Feb 26, 2023

Should I include changes on plugin-ideal-images if you want to preview images in tests?
Without that changes, I think we have to add complex Webpack settings to docusaurus.config to use lqip-loader directly for WebP/AVIF in Docusaurus and a custom type definition for those image types.

@slorber
Copy link
Collaborator

slorber commented Mar 2, 2023

Should I include changes on plugin-ideal-images if you want to preview images in tests? Without that changes, I think we have to add complex Webpack settings to docusaurus.config to use lqip-loader directly for WebP/AVIF in Docusaurus and a custom type definition for those image types.

I don't really understand. Is this PR adding support for webp/avif or not? What does "minimal support" even means? Does it mean it only add extension support but in practice it doesn't really work? 🤔

I don't want to merge a pr for a half-backed feature. We should either:

  • Have no support for webp/avif
  • Have support for webp/avif, and prove it by making it work on our own website + deploy previews

So far I only see a PR that has code changes that look fine, but no proof that the whole system works and enables users to use these image extensions in practice.

I need to see the entire thing in action. I need to see avif and webp images being used here: https://deploy-preview-8686--docusaurus-2.netlify.app/tests/

If that means modifying plugin-ideal-images then yes, it should be done on this PR 🤪 We don't want to ask users to "complete" the support themselves with complex Webpack config.

@tats-u
Copy link
Contributor Author

tats-u commented Mar 2, 2023

@slorber WebP/AVIF support consists of three phases:

  1. The current lqip-loader doesn't handle WebP/AVIF at all. The current patch of this PR allows it to possible to handle them without optimization (e.g. choose WebP/lossless lqip for AVIF/WebP source, or enable YUV444 for AVIF or sharp YUV for WebP)
  2. Allow plugin-ideal-image to integrate lqip-loader with responsive-loader (has already supported WebP/AVIF) by adding additional Webpack settings.
  3. (Additional) Improvement of lqip-loader to choose more optimal image types and settings for lqips.

I have planned to submit PRs per phase but it doesn't seem to be what you want.

If that means modifying plugin-ideal-images then yes, it should be done on this PR

You mean I should include (at least) 2. in this PR. I'll convert this PR to draft once.

@tats-u tats-u marked this pull request as draft March 2, 2023 15:44
@tats-u tats-u changed the title Add minimal support for WebP/AVIF in @docusaurus/lqip-loader Add essential support for WebP/AVIF in @docusaurus/lqip-loader and @docusaurus/plugin-ideal-image Mar 2, 2023
@slorber
Copy link
Collaborator

slorber commented Mar 2, 2023

What I'd like is to have this PR bring a benefit to the end user.

If you add support to a transitive dependency, but not the direct dependency, then there's no direct benefit for the user and we are not even 100% sure that the transitive dependency change will be good enough.

I'd prefer if you merged step 1 + step 2 and ensure that we deliver a new immediate feature to the users in a single PR, a feature that is battle-tested on our own site. It ensures that changes in both packages are working and play well together.

@tats-u
Copy link
Contributor Author

tats-u commented Mar 5, 2023

@slorber

I'd prefer if you merged step 1 + step 2 and ensure that we deliver a new immediate feature to the users in a single PR, a feature that is battle-tested on our own site. It ensures that changes in both packages are working and play well together.

I'll implement 1 & 2 in this PR.

@tats-u
Copy link
Contributor Author

tats-u commented Mar 5, 2023

Without applying this diff, I got a type error and don't know how to fix.

git diff packages/docusaurus-plugin-ideal-image/src/plugin-ideal-image.d.ts
diff --git a/packages/docusaurus-plugin-ideal-image/src/plugin-ideal-image.d.ts b/packages/docusaurus-plugin-ideal-image/src/plugin-ideal-image.d.ts
index dbf6ea2e83..e460efa62e 100644
--- a/packages/docusaurus-plugin-ideal-image/src/plugin-ideal-image.d.ts
+++ b/packages/docusaurus-plugin-ideal-image/src/plugin-ideal-image.d.ts
@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */

-import type {ImageWithLqip} from '@docusaurus/lqip-loader';
+// import type {ImageWithLqip} from '@docusaurus/lqip-loader';

 declare module '@docusaurus/plugin-ideal-image' {
   export type PluginOptions = {
@@ -75,7 +75,8 @@ declare module '@theme/IdealImage' {
     images: SrcType[];
   };

-  export type IdealImageEnabledSrc = ImageWithLqip<SrcImage>;
+  // export type IdealImageEnabledSrc = ImageWithLqip<SrcImage>;
+  export type IdealImageEnabledSrc = {preSrc: string; src: SrcImage};
   export type IdealImageSrc = IdealImageEnabledSrc | {default: string} | string;

   export interface Props extends ComponentProps<'img'> {
lerna info run Ran npm script 'build' in '@docusaurus/theme-search-algolia' in 2.3s:
$ tsc --build && node ../../admin/scripts/copyUntypedFiles.js && prettier --config ../../.prettierrc --write "lib/theme/**/*.js"
lib\theme\SearchBar\index.js 203ms
lib\theme\SearchPage\index.js 97ms
lib\theme\SearchTranslations\index.js 32ms
lerna ERR! yarn run build exited 1 in '@docusaurus/plugin-ideal-image'
lerna ERR! yarn run build stdout:
$ tsc --build && node ../../admin/scripts/copyUntypedFiles.js && prettier --config ../../.prettierrc --write "lib/theme/**/*.js"
src/theme/IdealImage/index.tsx(15,26): error TS2307: Cannot find module '@theme/IdealImage' or its corresponding type declarations.
src/theme/IdealImage/index.tsx(101,35): error TS7006: Parameter 'image' implicitly has an 'any' type.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
lerna ERR! yarn run build stderr:
error Command failed with exit code 1.
lerna ERR! yarn run build exited 1 in '@docusaurus/plugin-ideal-image'
lerna WARN complete Waiting for 1 child process to exit. CTRL-C to exit immediately.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@tats-u
Copy link
Contributor Author

tats-u commented Mar 5, 2023

Additional constrains:

  • Lossless WebPs are converted to lossy ones and get their colors broken (YUV420).
  • sharp YUV (smartSubsample) is not used
  • JPEG & AVIF chroma subsampling is not retained (420 for JPEG / 444 for AVIF forced)

3 is more important than I think.

@slorber
Copy link
Collaborator

slorber commented Mar 8, 2023

I'd like to have avif/webp support but have to focus on something else and am not familiar with that part of the codebase (coded a very long time ago by someone else).

I don't know about the TS issue.

Unfortunately, I can't really help you right now. Please try to figure this out otherwise we'll come back to it later.

@tats-u tats-u force-pushed the lqip-web-avif branch 2 times, most recently from 522ab86 to f0c2e40 Compare March 11, 2023 16:14
@slorber
Copy link
Collaborator

slorber commented Apr 13, 2023

are you able to make it work, or should we close?

@tats-u
Copy link
Contributor Author

tats-u commented Apr 13, 2023

Just I have no time until the next week. Sorry.

@tats-u
Copy link
Contributor Author

tats-u commented Apr 20, 2023

libvips, which sharp depends on, doesn't provide a metadata to tell WebP lossless from WebP lossy.
This disguises me from working on this.

@tats-u
Copy link
Contributor Author

tats-u commented Apr 20, 2023

I get this error only when yarn run build (not yarn run start)

TypeError: _chalk.default.bold is not a function

I will try rebasing later.

@slorber
Copy link
Collaborator

slorber commented Apr 20, 2023

I fixed this error recently in main, it hides another SSR error but you will need to rebase to get the real error message.

`sharp` does support both.
Generated lqips for WebP/AVIF have the same formats as inputs, but different formats may be better. (needs improvement if so)
- PNG is from `website/static/img`
- Convert it to WebP/AVIF using Squoosh with max effort
- AVIF: use YUV444; quality = 30
- WebP: use lossless
@tats-u
Copy link
Contributor Author

tats-u commented Apr 22, 2023

yarn run start succeeds but yarn run build fails.
I can't find out which src is not set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants