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

fix: do not use ideal image plugin in dev env #5540

Merged
merged 2 commits into from
Sep 23, 2021
Merged

fix: do not use ideal image plugin in dev env #5540

merged 2 commits into from
Sep 23, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Sep 8, 2021

Motivation

Should fix #4588

Currently, images that are rendered through IdealImage component will become blurry (or more precisely, placeholders is displayed instead them).
The reason for this is the component expects to get resized images via responsive-loader, but in dev env we can't have build artifacts, so there are no generated images, and their placeholders are always displayed instead.
Therefore during development I propose just use regular img element which will render original image (received as module).

Have you read the Contributing Guidelines on pull requests?

Preview

Test Plan

Start dev server locally and open the Showcase page.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Sep 8, 2021
@lex111 lex111 requested a review from slorber as a code owner September 8, 2021 12:48
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 8, 2021
@netlify
Copy link

netlify bot commented Sep 8, 2021

✔️ [V2]

🔨 Explore the source changes: 63fad93

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/614b8a1a2eb5db000783f426

😎 Browse the preview: https://deploy-preview-5540--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Sep 8, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 92
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5540--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Sep 8, 2021

Size Change: -4 B (0%)

Total Size: 825 kB

ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 36.7 kB 0 B
website/build/assets/css/styles.********.css 94 kB 0 B
website/build/assets/js/main.********.js 414 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 67 kB 0 B
website/build/blog/index.html 38 kB 0 B
website/build/docs/index.html 44.5 kB -2 B (0%)
website/build/docs/installation/index.html 52.7 kB -2 B (0%)
website/build/index.html 30.8 kB 0 B
website/build/tests/docs/index.html 25.3 kB 0 B
website/build/tests/docs/standalone/index.html 22.9 kB 0 B

compressed-size-action

@Irev-Dev
Copy link

Irev-Dev commented Sep 8, 2021

I'm in favour of this approach. It's a good compromise thats much better than blurry images in dev.

@Josh-Cena
Copy link
Collaborator

Note: if the theme/IdealImage.js file is changed to TS, it needs to be transpiled twice like in theme-classic so that there's a JS version for swizzling

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.

Disabling the ideal image in dev could do the trick. Not 100% sure it's a good idea that will not have any weird side effects, but we can give it a try.

I suggest we implement some utils to compile themes to lib + lib-next first (as suggested by @Josh-Cena here #5564) so that this PR is easier to implement properly with goof swizzling support

const {alt, className, img} = props;

// In dev env just use regular img with original file
if (img.default) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it's a very reliable test.
In the future (Webpack 5 asset pipeline, new URL() etc...) it may be possible that both won't have a default (but we can probably improve this test later anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of which, what is the progress in #4708?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Standby for now, not a top priority atm but I'd like to make this work someday 😅

return {
name: 'docusaurus-plugin-ideal-image',

getThemePath() {
return path.resolve(__dirname, './theme');
return path.resolve(__dirname, '../lib/theme');
Copy link
Collaborator

Choose a reason for hiding this comment

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

as @Josh-Cena said we should have a lib-next output folder otherwise users swizzling this comp will get a bad output, + we should add a getTypeScriptThemePath() to swizzle this comp with --typescript option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did not assume that this component would be available for swizzling. That's why I rolled back the changes related with the migration to TS. Better to do it in separate PR, right now we don't have proper infrastructure set up for it.

@slorber
Copy link
Collaborator

slorber commented Sep 23, 2021

Thanks, LGTM 👍 hope it won't be annoying for anyone, let's see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ideal-image: support non-blurry images in dev
5 participants