-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
base: main
Are you sure you want to change the base?
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this 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
?
@slorber Could you tell me what kind of tests I should add? I have no idea. |
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 |
@slorber No materials are present now and this modification itself doesn't affect on Docusaurus without additional Webpack settings. The
|
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/ |
Should I include changes on |
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:
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 |
@slorber WebP/AVIF support consists of three phases:
I have planned to submit PRs per phase but it doesn't seem to be what you want.
You mean I should include (at least) 2. in this PR. I'll convert this PR to draft once. |
@docusaurus/lqip-loader
@docusaurus/lqip-loader
and @docusaurus/plugin-ideal-image
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. |
I'll implement 1 & 2 in this PR. |
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'> {
|
Additional constrains:
3 is more important than I think. |
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. |
522ab86
to
f0c2e40
Compare
are you able to make it work, or should we close? |
Just I have no time until the next week. Sorry. |
|
I get this error only when
I will try rebasing later. |
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
|
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
Motivation
sharp
used in@docusaurus/lqip-loader
does support WebP/AVIF, but@docusaurus/lqip-loader
claims that it does not support them. This preventsplugin-ideal-image
from supporting WebP/AVIF.This PR is to add minimal support (may be improved later) for them.
Test Plan
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.
AVIF's lqip may be so large that we have to use WebP instead.
Blocked by libvips / sharp