Add new prefer-ideal-image eslint rule #8826

Devansu-Yadav · 2023-03-25T14:37:27Z

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

Described in #6472. Basically, this plugin enforces the use of @theme/IdealImage component instead of <img> tags.

Test Plan

Added tests using eslint's RuleTester utility.

Test links

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

Related issues/PRs

#6472

Devansu-Yadav · 2023-03-25T14:40:05Z

@slorber I still need to fix linting errors and add documentation for this eslint rule, but just wanted to ask, do you think it would be useful to implement auto-fixing capability for this rule?

netlify · 2023-03-25T14:41:15Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`678f850`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/641f072ae987810008319000
😎 Deploy Preview	https://deploy-preview-8826--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle 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 · 2023-03-25T14:41:47Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 59	🟢 97	🟢 100	🟢 100	🟠 89	Report
/docs/installation	🟠 80	🟢 100	🟢 100	🟢 100	🟠 89	Report

Josh-Cena · 2023-03-25T14:51:36Z

packages/eslint-plugin/src/index.ts

@@ -16,6 +16,7 @@ export = {
        '@docusaurus/string-literal-i18n-messages': 'error',
        '@docusaurus/no-html-links': 'warn',
        '@docusaurus/prefer-docusaurus-heading': 'warn',
+        '@docusaurus/prefer-ideal-image': 'warn',


This should be off by default. Not many use ideal image. It should be off in our own codebase except the website as well.

Devansu-Yadav · 2023-03-27T16:10:07Z

@slorber I still need to fix linting errors and add documentation for this eslint rule, but just wanted to ask, do you think it would be useful to implement auto-fixing capability for this rule?

@Josh-Cena What do you think about this?

Josh-Cena · 2023-03-27T16:12:58Z

I don't think we need auto-fix, no. It sounds very non-trivial and has a high risk of breaking otherwise valid code. Auto-fixing should never change code behavior.

Devansu-Yadav · 2023-03-28T07:41:24Z

@slorber @Josh-Cena Seems like some of the checks are failing for this PR because of the following error:

12:50:33 PM: [INFO] [en] Creating an optimized production build...
12:50:50 PM: [info] [webpackbar] Compiling Client
12:50:50 PM: [info] [webpackbar] Compiling Server
12:51:02 PM: [success] [webpackbar] Client: Compiled successfully in 11.76s
12:51:20 PM: [success] [webpackbar] Server: Compiled with some errors in 30.14s
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: TypeError: _chalk.default.bold is not a function
12:51:20 PM: [ERROR] Unable to build website for locale en.
12:51:20 PM: [ERROR] Error: Failed to compile with errors.
12:51:20 PM:     at /opt/build/repo/packages/docusaurus/lib/webpack/utils.js:180:24
12:51:20 PM:     at /opt/build/repo/node_modules/webpack/lib/MultiCompiler.js:554:14
12:51:20 PM:     at processQueueWorker (/opt/build/repo/node_modules/webpack/lib/MultiCompiler.js:491:6)
12:51:20 PM:     at processTicksAndRejections (node:internal/process/task_queues:78:11)

Anything that needs to be done from my side to fix this?

slorber

Thanks but unfortunately that's not so simple. It is preferable that you open an RFC of the behavior you plan to implement before starting the work because the description of the original issue is not really complete and does not mention any design or edge cases to consider.

prefer-ideal-image: ensures @theme/IdealImage is used instead of img tags

We should allow any string link coming from props, dynamic or hardcoded values:

<img src="https://github.com/xyz.png" />

<img src={`${props.githubUrl}.png`} />

<img src="./xyz.png" />

<img src={useBaseUrl("./xyz.png"} />


const imgSrc = "./xyz.png"
<img src={imgSrc} />

Eventually, add an option to reject relative local links?

The only patterns that IMHO we can safely reject are the usage of images that are required/imported locally:

import imgSrc from './path/to/img.png';

<img src={imgSrc} />

<img src={require("./img.png")}/>

The weird chalk build errors you get are a bug that I will fix.

You can get the real errors by turning disableInDev: false, in the plugin options

https://docusaurus.io/docs/api/plugins/@docusaurus/plugin-ideal-image#disableInDev

err TypeError: Cannot read properties of undefined (reading 'src')
    at fallbackParams (main:8332:43)
    at IdealImage.render (main:7750:106)
    at d (main:67040:231)
    at bb (main:67041:16)
    at a.b.render (main:67047:43)
    at a.b.read (main:67046:83)
    at Object.exports.renderToString (main:67057:138)
    at doRender (main:219469:37)
    at async render (main:219387:16)
    at async /Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/p-map/index.js:57:22

This PR is not ready to merge, not because of the CI failures, but because the behavior implemented is not what we need.

slorber · 2023-04-06T14:29:52Z

packages/eslint-plugin/src/rules/__tests__/prefer-ideal-image.test.ts

+  ],
+  invalid: [
+    {
+      code: "<img src='./path/to/img.png' alt='some alt text' />",


should not be rejected?

slorber · 2023-04-06T14:30:08Z

packages/eslint-plugin/src/rules/__tests__/prefer-ideal-image.test.ts

+      code: "<img src='./path/to/img.png' srcset='./path/to/img-480w.jpg 480w, ./path/to/img-800w.png 800w' sizes='(max-width: 600px) 480px, 800px' alt='some alt text' />",
+      errors: errorsJSX,


should not be rejected?

slorber · 2023-04-06T14:31:45Z

website/src/plugins/changelog/theme/ChangelogItem/Header/Author/index.tsx

-          <img
+          <Image
            className={styles.image}
-            src={imageURL}
-            alt={name}
-            onError={(e) => {
-              // Image returns 404 if the user's handle changes. We display a
-              // fallback instead.
-              e.currentTarget.src =
-                'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="600" height="600" fill="none" stroke="%2325c2a0" stroke-width="30" version="1.1"><circle cx="300" cy="230" r="115"/><path stroke-linecap="butt" d="M106.81863443903,481.4 a205,205 1 0,1 386.36273112194,0"/></svg>';
+            img={{
+              src: {
+                src: imageURL,
+                preSrc:
+                  'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="600" height="600" fill="none" stroke="%2325c2a0" stroke-width="30" version="1.1"><circle cx="300" cy="230" r="115"/><path stroke-linecap="butt" d="M106.81863443903,481.4 a205,205 1 0,1 386.36273112194,0"/></svg>',
+                images: [],
+              },
+              preSrc:
+                'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="600" height="600" fill="none" stroke="%2325c2a0" stroke-width="30" version="1.1"><circle cx="300" cy="230" r="115"/><path stroke-linecap="butt" d="M106.81863443903,481.4 a205,205 1 0,1 386.36273112194,0"/></svg>',
            }}
+            alt={name}
+            // onError={(e) => {
+            //   // Image returns 404 if the user's handle changes. We display a
+            //   // fallback instead.
+            //   e.currentTarget.src =
+            //     'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" width="600" height="600" fill="none" stroke="%2325c2a0" stroke-width="30" version="1.1"><circle cx="300" cy="230" r="115"/><path stroke-linecap="butt" d="M106.81863443903,481.4 a205,205 1 0,1 386.36273112194,0"/></svg>';
+            // }}


that seems overly complicated, using <img> is fine in this case IMHO

slorber · 2023-04-06T14:34:25Z

website/src/pages/index.tsx

+      <Image
        className={styles.featureImage}
        alt={feature.title}
        width={Math.floor(feature.image.width)}
        height={Math.floor(feature.image.height)}
-        src={withBaseUrl(feature.image.src)}
+        img={withBaseUrl(feature.image.src)}


the ideal image plugin is not designed to work with SVGs

Eventually we could import the svgs and use them as components instead of using <img src="xyz.svg"/> like we do atm

the ideal image plugin is not designed to work with SVGs

I had noticed that it only works with JPEGs and PNGs but somehow completely missed out in this case 😅

Eventually we could import the svgs and use them as components instead of using <img src="xyz.svg"/> like we do atm

@slorber Should I address this change in this PR?

slorber · 2023-04-06T14:34:35Z

website/src/pages/index.tsx

-          <img
+          <Image
            alt={translate({message: 'Docusaurus with Keytar'})}
            className={styles.heroLogo}
-            src={useBaseUrl('/img/docusaurus_keytar.svg')}
+            img={useBaseUrl('/img/docusaurus_keytar.svg')}


the ideal image plugin is not designed to work with SVGs

Eventually we could import the svgs and use them as components instead of using <img src="xyz.svg"/> like we do atm

slorber · 2023-04-06T14:35:41Z

website/src/components/TweetQuote/index.tsx

+            <Image
              alt={name}
              className={clsx('avatar__photo', styles.avatarImg)}
-              src={avatar}
+              img={avatar}


Ideal image is not designed to work with external URLs. <img> is fine in this case

slorber · 2023-04-06T14:35:53Z

website/src/components/Tweet/index.tsx

-          <img
+          <Image
            alt={name}
            className="avatar__photo"
-            src={`https://unavatar.io/twitter/${handle}?fallback=https://github.com/${githubUsername}.png`}
+            img={`https://unavatar.io/twitter/${handle}?fallback=https://github.com/${githubUsername}.png`}


Ideal image is not designed to work with external URLs. <img> is fine in this case

slorber · 2023-04-06T14:36:05Z

website/src/components/TeamProfileCards/index.tsx

-            <img
+            <Image
              className="avatar__photo avatar__photo--xl"
-              src={`${githubUrl}.png`}
+              img={`${githubUrl}.png`}


Ideal image is not designed to work with external URLs. <img> is fine in this case

slorber · 2023-04-06T14:36:11Z

website/src/components/ProductHuntCard.tsx

-      <img
-        src="https://api.producthunt.com/widgets/embed-image/v1/featured.svg?post_id=353916&theme=light"
+      <Image
+        img="https://api.producthunt.com/widgets/embed-image/v1/featured.svg?post_id=353916&theme=light"


Ideal image is not designed to work with external URLs. <img> is fine in this case

Devansu-Yadav · 2023-04-08T14:32:06Z

Thanks but unfortunately that's not so simple. It is preferable that you open an RFC of the behavior you plan to implement before starting the work because the description of the original issue is not really complete and does not mention any design or edge cases to consider.

@slorber Apologies for underestimating the complexity of the implementation and misunderstanding the behavior of this eslint rule. I should have discussed the schema and the desired behavior first before implementing this rule 😅

I agree the original issue does lack more details regarding the design and edge cases. Actually, I saw one of the comments on the original issue that mentioned that we could directly link a PR back to this issue; hence I went ahead with implementing this PR.

We should allow any string link coming from props, dynamic or hardcoded values:

Sure, it makes sense, as I noticed that the internal implementation of IdealImage in Docusaurus just returns an <img> tag whenever we pass string links.

Eventually, add an option to reject relative local links?

Any particular reason why we would want to reject relative local links?

The only patterns that IMHO we can safely reject are the usage of images that are required/imported locally:

Got it 👍. So, do we want to encourage users to use the <IdealImage /> whenever images are required/imported locally? Also, if we encourage this behavior, do we also want to check whether the imported/required image is of a supported format or not and throw a violation through this rule?

You can get the real errors by turning disableInDev: false, in the plugin options

https://docusaurus.io/docs/api/plugins/@docusaurus/plugin-ideal-image#disableInDev

err TypeError: Cannot read properties of undefined (reading 'src')
    at fallbackParams (main:8332:43)
    at IdealImage.render (main:7750:106)
    at d (main:67040:231)
    at bb (main:67041:16)
    at a.b.render (main:67047:43)
    at a.b.read (main:67046:83)
    at Object.exports.renderToString (main:67057:138)
    at doRender (main:219469:37)
    at async render (main:219387:16)
    at async /Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/p-map/index.js:57:22

Got it 👍

This PR is not ready to merge, not because of the CI failures, but because the behavior implemented is not what we need.

I'll make the appropriate changes in this PR to fulfill the desired behavior of this eslint rule.

slorber · 2023-04-13T12:55:33Z

@slorber Apologies for underestimating the complexity of the implementation and misunderstanding the behavior of this eslint rule. I should have discussed the schema and the desired behavior first before implementing this rule 😅

I agree the original issue does lack more details regarding the design and edge cases. Actually, I saw one of the comments on the original issue that mentioned that we could directly link a PR back to this issue; hence I went ahead with implementing this PR.

Not a big deal, some eslint rules are simples while others are much more complex. We can continue the design of the feature in this PR.

We should allow any string link coming from props, dynamic or hardcoded values:

Sure, it makes sense, as I noticed that the internal implementation of IdealImage in Docusaurus just returns an <img> tag whenever we pass string links.

That's not what I wanted to say. What I mean is that this should be valid because the user could use this component using absolute URLs like a github avatar link.

function Avatar({src}) {
  return <img src={src}/>
}

We can't know if the component is meant to be used with local images (that can be required) so it shouldn't error by default. At most it should be a warning IMHO, but probably default to disabled.

Eventually, add an option to reject relative local links?

Any particular reason why we would want to reject relative local links?

If an image is in the repo, it's probably more optimized to require it instead of using a string path.

I think it's good if we emit a warning by default (with option to disable) for these cases:

<img src="./xyz.png" />

<img src="/xyz.png" />

const imgSrc = "./xyz.png"
<img src={imgSrc} />

const imgSrc = "/xyz.png"
<img src={imgSrc} />

The only patterns that IMHO we can safely reject are the usage of images that are required/imported locally:

Got it 👍. So, do we want to encourage users to use the <IdealImage /> whenever images are required/imported locally?

Yes, by default we should error for these patterns and encourage usage of the <IdealImage/> component:

import imgSrc from './path/to/img.png';

<img src={imgSrc} />

<img src={require("./img.png")}/>

Also, if we encourage this behavior, do we also want to check whether the imported/required image is of a supported format or not and throw a violation through this rule?

You mean requiring avif, webp and other similar extensions?

I guess it's not really necessary, the Webpack loader will likely already throw an error and we'll likely want to support those extensions in the future.

I'll make the appropriate changes in this PR to fulfill the desired behavior of this eslint rule.

Thanks

I'm not very skilled in writing ESLint plugins, so can't really help to figure out how to check if image was locally required/imported. Maybe @Josh-Cena can guide you?

Signed-off-by: Devansu <[email protected]>

Devansu-Yadav requested review from slorber, lex111 and Josh-Cena as code owners March 25, 2023 14:37

facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 25, 2023

Josh-Cena reviewed Mar 25, 2023

View reviewed changes

Devansu-Yadav force-pushed the ideal-image-plugin branch from db99c65 to 1ffcfec Compare March 28, 2023 07:18

Devansu-Yadav force-pushed the ideal-image-plugin branch from 1ffcfec to 240f1b4 Compare April 4, 2023 06:05

slorber requested changes Apr 6, 2023

View reviewed changes

slorber marked this pull request as draft April 6, 2023 14:40

slorber mentioned this pull request Apr 7, 2023

fix(core): better error logging on SSR/dev failures + log stacktraces and error causes #8872

Merged

Devansu-Yadav added 5 commits June 25, 2023 15:54

feat: add test & configure prefer-ideal-image rule

51f9a79

Signed-off-by: Devansu <[email protected]>

feat: add prefer-ideal-image eslint rule

c239d1c

Signed-off-by: Devansu <[email protected]>

fix: linting errors and update eslint config

b28b8fd

Signed-off-by: Devansu <[email protected]>

docs: add docs for prefer-ideal-image rule

6016b36

Signed-off-by: Devansu <[email protected]>

minor: update eslint config

78a6a25

Signed-off-by: Devansu <[email protected]>

Devansu-Yadav force-pushed the ideal-image-plugin branch from 240f1b4 to 78a6a25 Compare June 25, 2023 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new prefer-ideal-image eslint rule #8826

Add new prefer-ideal-image eslint rule #8826

Devansu-Yadav commented Mar 25, 2023

Devansu-Yadav commented Mar 25, 2023

netlify bot commented Mar 25, 2023

github-actions bot commented Mar 25, 2023 •

edited

Loading

Josh-Cena Mar 25, 2023

Devansu-Yadav commented Mar 27, 2023 •

edited

Loading

Josh-Cena commented Mar 27, 2023

Devansu-Yadav commented Mar 28, 2023 •

edited

Loading

slorber left a comment

slorber Apr 6, 2023

slorber Apr 6, 2023

slorber Apr 6, 2023

slorber Apr 6, 2023

Devansu-Yadav Apr 8, 2023 •

edited

Loading

slorber Apr 6, 2023

slorber Apr 6, 2023

slorber Apr 6, 2023

slorber Apr 6, 2023

slorber Apr 6, 2023

Devansu-Yadav commented Apr 8, 2023 •

edited

Loading

slorber commented Apr 13, 2023 •

edited

Loading

		code: "<img src='./path/to/img.png' srcset='./path/to/img-480w.jpg 480w, ./path/to/img-800w.png 800w' sizes='(max-width: 600px) 480px, 800px' alt='some alt text' />",
		errors: errorsJSX,

Add new prefer-ideal-image eslint rule #8826

Are you sure you want to change the base?

Add new prefer-ideal-image eslint rule #8826

Conversation

Devansu-Yadav commented Mar 25, 2023

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

Devansu-Yadav commented Mar 25, 2023

netlify bot commented Mar 25, 2023

✅ [V2]

github-actions bot commented Mar 25, 2023 • edited Loading

⚡️ Lighthouse report for the deploy preview of this PR

Choose a reason for hiding this comment

Devansu-Yadav commented Mar 27, 2023 • edited Loading

Josh-Cena commented Mar 27, 2023

Devansu-Yadav commented Mar 28, 2023 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devansu-Yadav Apr 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devansu-Yadav commented Apr 8, 2023 • edited Loading

slorber commented Apr 13, 2023 • edited Loading

github-actions bot commented Mar 25, 2023 •

edited

Loading

Devansu-Yadav commented Mar 27, 2023 •

edited

Loading

Devansu-Yadav commented Mar 28, 2023 •

edited

Loading

Devansu-Yadav Apr 8, 2023 •

edited

Loading

Devansu-Yadav commented Apr 8, 2023 •

edited

Loading

slorber commented Apr 13, 2023 •

edited

Loading