chore: fix CSS import order ESLint rule #7852

MarkShawn2020 · 2022-07-30T16:04:04Z

enhance: added ESLint ruler for CSS import

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

In previous PR #7849, I found there is a bug when importing a CSS file after importing a type from @docusaurus/type.

At first, we (with the maintainer) are guessing the problem is caused by my system environment. But later I did a lot of checks, and came to realize that it's not.

Best Solution (potential)

Just adding one extra ruler in order to force CSS to be imported at last:

// .eslintrc.js
{
  pattern: '*.css', // or '*.+(css|sass|less|scss|pcss|styl)' for general match
  patternOptions: {matchBase: true},
  group: 'unknown',
  position: 'after',
}

Test (tested both in WebStorm and VSCode)

As mentioned from #7849, we can do a simple test.

TIPS:

grep -irE "import.*\.css" packages | cut -d ':' -f 1 | uniq can list all the files with css files imported.
the target type file is at packages/docusaurus-types/src/index.d.ts

Take the packages/docusaurus-theme-common/src/components/Details/index.tsx as an example, we can easily get the warning if we import types from @docusaurus/types before css file.

However, if we omitted the imported default variable, then the warning would go away immediately (but would cause the later reference problems):

Essential

The plugin of eslint-plugin-import has no default type check for file extensions, so:

grammar	group interpreated
`import css from 'xx.css'`	`internal`
`import css from './xx.css'`	`sibling`
`import css from '../xx.css'`	`parent`
`import '../xx.css'`	`unknown`(maybe)

Hence, in normal situation, those internal | sibling | parent would be ranked in front of type, except we explictly declaring the module order, just like what the former work has been done:

// .eslintrc.js
...
{pattern: '@theme/**', group: 'internal'},
{pattern: '@site/**', group: 'internal'},
{pattern: '@theme-init/**', group: 'internal'},
{pattern: '@theme-original/**', group: 'internal'},
...

All those variables, as well as types, would be intepreted before any css-like imports.

But then, not so lucky for those types imported from @docusaurus/types.

Related issues/PRs

#7849

References

You can get more info from the following references:

official documentation of eslint-plugin-import: https://github.com/import-js/eslint-plugin-import/blob/v2.26.0/docs/rules/order.md
a discussion on how to import css at last (source of the potential best solution): Add extensions order support for import/order import-js/eslint-plugin-import#1239

netlify · 2022-07-30T16:06:46Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`d92db38`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/62e64a8234ca16000a486c3b
😎 Deploy Preview	https://deploy-preview-7852--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 · 2022-07-30T16:07:14Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 62	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 77	🟢 100	🟢 100	🟢 100	🟢 90	Report

Josh-Cena

Thanks—can you fix existing files so we can take a look at the effect?

MarkShawn2020 · 2022-07-31T02:43:21Z

Thanks—can you fix existing files so we can take a look at the effect?

Take easy, I am the first one in trouble of this problem, so there is no existing file need to be fixed.

Josh-Cena · 2022-07-31T02:46:16Z

Try running yarn lint?

$ yarn lint
yarn run v1.22.18
$ yarn lint:js && yarn lint:style && yarn lint:spelling
$ eslint --cache --report-unused-disable-directives "**/*.{js,jsx,ts,tsx,mjs}"

docusaurus/packages/docusaurus-mdx-loader/src/remark/admonitions/index.ts
  61:33  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  61:43  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-classic/src/theme/CodeBlock/Container/index.tsx
  23:21  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-classic/src/theme/DocCard/index.tsx
  20:1  warning  `@docusaurus/plugin-content-docs` import should occur before import of `./styles.module.css`  import/order

docusaurus/packages/docusaurus-theme-classic/src/theme/NavbarItem/index.tsx
  27:45  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-common/src/contexts/blogPost.tsx
  27:1  warning  This line has a comment length of 84. Maximum allowed is 80  max-len

docusaurus/packages/docusaurus-theme-live-codeblock/src/theme/Playground/index.tsx
  19:1  warning  `@docusaurus/theme-live-codeblock` import should occur before import of `./styles.module.css`  import/order

docusaurus/packages/docusaurus-theme-search-algolia/src/theme/SearchPage/index.tsx
  35:1  warning  `@docusaurus/theme-search-algolia` import should occur before import of `./styles.module.css`  import/order

docusaurus/website/docusaurus.config.js
  91:7  warning  Do not nest ternary expressions  no-nested-ternary

✖ 9 problems (0 errors, 9 warnings)
  0 errors and 3 warnings potentially fixable with the `--fix` option.

MarkShawn2020 · 2022-07-31T02:47:42Z

Try running yarn lint?

$ yarn lint
yarn run v1.22.18
$ yarn lint:js && yarn lint:style && yarn lint:spelling
$ eslint --cache --report-unused-disable-directives "**/*.{js,jsx,ts,tsx,mjs}"

docusaurus/packages/docusaurus-mdx-loader/src/remark/admonitions/index.ts
  61:33  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  61:43  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-classic/src/theme/CodeBlock/Container/index.tsx
  23:21  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-classic/src/theme/DocCard/index.tsx
  20:1  warning  `@docusaurus/plugin-content-docs` import should occur before import of `./styles.module.css`  import/order

docusaurus/packages/docusaurus-theme-classic/src/theme/NavbarItem/index.tsx
  27:45  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

docusaurus/packages/docusaurus-theme-common/src/contexts/blogPost.tsx
  27:1  warning  This line has a comment length of 84. Maximum allowed is 80  max-len

docusaurus/packages/docusaurus-theme-live-codeblock/src/theme/Playground/index.tsx
  19:1  warning  `@docusaurus/theme-live-codeblock` import should occur before import of `./styles.module.css`  import/order

docusaurus/packages/docusaurus-theme-search-algolia/src/theme/SearchPage/index.tsx
  35:1  warning  `@docusaurus/theme-search-algolia` import should occur before import of `./styles.module.css`  import/order

docusaurus/website/docusaurus.config.js
  91:7  warning  Do not nest ternary expressions  no-nested-ternary

✖ 9 problems (0 errors, 9 warnings)
  0 errors and 3 warnings potentially fixable with the `--fix` option.

After I posted that command, I found there are some missing scenario, I do find one problem in packages/docusaurus-theme-classic/src/nprogress.ts:

MarkShawn2020 · 2022-07-31T02:52:54Z

ackages/docusaurus-theme-classic/src/theme/CodeBlock/Container/index.tsx
23:21 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any

laugh, I guess I have did yarn lint yesterday based on fixed .eslintrc.js, no wonder I can't find more problems....

Josh-Cena · 2022-07-31T02:52:58Z

There are 3 warnings reported by import/order. Those warnings are introduced by this change. They need to be fixed before we can merge this.

MarkShawn2020 · 2022-07-31T02:56:36Z

There are 3 warnings reported by import/order. Those warnings are introduced by this change. They need to be fixed before we can merge this.

The 3 warnings haved been fixed in my local env, and I am thinking about the problem in packages/docusaurus-theme-classic/src/nprogress.ts since the order between type and import "./xxx.css" is insensitive.

…iles

MarkShawn2020 · 2022-07-31T04:35:03Z

There are 3 warnings reported by import/order. Those warnings are introduced by this change. They need to be fixed before we can merge this.

see d4ed946, all the problems should have been resolved, with an extra fix of copyright header problems involved with css files.

packages/stylelint-copyright/src/index.ts

Josh-Cena

Thank you! This looks much greater than before :D

enhance: added ESLint ruler for CSS import

67b5f86

MarkShawn2020 requested review from slorber, lex111 and Josh-Cena as code owners July 30, 2022 16:04

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 30, 2022

Josh-Cena changed the title ~~enhance: added ESLint ruler for CSS import~~ chore: fix CSS import order ESLint rule Jul 31, 2022

Josh-Cena added the pr: internal This PR does not touch production code, or is not meaningful enough to be in the changelog. label Jul 31, 2022

Josh-Cena requested changes Jul 31, 2022

View reviewed changes

chore: added ruler for default import of css, and fixed all exsting f…

d4ed946

…iles

MarkShawn2020 force-pushed the pr-eslint branch from 0a4b0a5 to d4ed946 Compare July 31, 2022 04:31

Josh-Cena reviewed Jul 31, 2022

View reviewed changes

packages/stylelint-copyright/src/index.ts Outdated Show resolved Hide resolved

chore: reverted changes on css header rulers

d92db38

Josh-Cena approved these changes Jul 31, 2022

View reviewed changes

Josh-Cena merged commit f683589 into facebook:main Jul 31, 2022

MarkShawn2020 deleted the pr-eslint branch July 31, 2022 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix CSS import order ESLint rule #7852

chore: fix CSS import order ESLint rule #7852

MarkShawn2020 commented Jul 30, 2022 •

edited

Loading

netlify bot commented Jul 30, 2022 •

edited

Loading

github-actions bot commented Jul 30, 2022 •

edited

Loading

Josh-Cena left a comment

MarkShawn2020 commented Jul 31, 2022

Josh-Cena commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022

Josh-Cena commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022 •

edited

Loading

MarkShawn2020 commented Jul 31, 2022

Josh-Cena left a comment

chore: fix CSS import order ESLint rule #7852

chore: fix CSS import order ESLint rule #7852

Conversation

MarkShawn2020 commented Jul 30, 2022 • edited Loading

enhance: added ESLint ruler for CSS import

Pre-flight checklist

Motivation

Best Solution (potential)

Other Solutions

Test (tested both in WebStorm and VSCode)

Essential

Other Problems

Related issues/PRs

References

netlify bot commented Jul 30, 2022 • edited Loading

✅ [V2]

github-actions bot commented Jul 30, 2022 • edited Loading

⚡️ Lighthouse report for the deploy preview of this PR

Josh-Cena left a comment

Choose a reason for hiding this comment

MarkShawn2020 commented Jul 31, 2022

Josh-Cena commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022

Josh-Cena commented Jul 31, 2022

MarkShawn2020 commented Jul 31, 2022 • edited Loading

MarkShawn2020 commented Jul 31, 2022

Josh-Cena left a comment

Choose a reason for hiding this comment

MarkShawn2020 commented Jul 30, 2022 •

edited

Loading

netlify bot commented Jul 30, 2022 •

edited

Loading

github-actions bot commented Jul 30, 2022 •

edited

Loading

MarkShawn2020 commented Jul 31, 2022 •

edited

Loading