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.

Sign up for GitHub

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

feat(eslint-plugin): add autofixable import rules #22050

Closed
wants to merge 4 commits into from
Closed

feat(eslint-plugin): add autofixable import rules #22050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb72b82
feat(eslint-plugin): enable autofixable import rules
Hotell Mar 10, 2022
9f7dd8e
chore: run eslint --fix on save
Hotell Mar 10, 2022
cc5df72
package change files
Hotell Mar 10, 2022
33e3181
Update change/@fluentui-eslint-plugin-e6e0684b-1ee7-4695-a151-9f716d7…
Hotell Mar 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
// When opening a file, `editor.tabSize` and `editor.insertSpaces` will NOT be detected based on the file contents.
"editor.detectIndentation": false,
"editor.formatOnSave": true,
"editor.codeActionsOnSave": {
"source.fixAll.eslint": true
},
// When enabled, will trim trailing whitespace when you save a file.
"files.trimTrailingWhitespace": true,
"files.insertFinalNewline": true,
Expand Down Expand Up @@ -45,6 +48,7 @@
},
"editor.rulers": [120],
"eslint.workingDirectories": [{ "mode": "auto" }], // infer working directory based on .eslintrc/package.json location
"eslint.codeActionsOnSave.mode": "problems",
"files.associations": {
"**/package.json.hbs": "json",
"**/*.json.hbs": "jsonc"
Expand Down
7 changes: 7 additions & 0 deletions change/@fluentui-eslint-plugin-e6e0684b-1ee7-4695-a151-9f716d78ab67.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "enable autofixable import rules",
"packageName": "@fluentui/eslint-plugin",
"email": "[email protected]",
"dependentChangeType": "none"
}
251 changes: 150 additions & 101 deletions packages/eslint-plugin/src/configs/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,31 +68,11 @@ const config = {
'**/*.scss.ts',
],
rules: {
'@rnx-kit/no-export-all': ['error', { expand: 'external-only' }],
'@fluentui/no-global-react': 'error',
'@fluentui/max-len': [
'error',
{
ignorePatterns: [
'require(<.*?>)?\\(',
'https?:\\/\\/',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of these changes is just re-ordering/collocation of rules

'^(import|export) ',
'^\\s+(<path )?d=',
'!raw-loader',
'\\bdata:image/',
],
max: 120,
},
],
'@fluentui/no-tslint-comments': 'error',

// tslint: function-name, variable-name
...configHelpers.getNamingConventionRule(false /* prefixWithI */),

'@typescript-eslint/no-empty-function': 'error',
'@typescript-eslint/no-explicit-any': 'error', // tslint: no-any
'@typescript-eslint/prefer-namespace-keyword': 'error',

/**
*
* core eslint rules
* @see https://eslint.org/docs/rules
*/
curly: ['error', 'all'],
'dot-notation': 'error',
eqeqeq: ['error', 'always'],
Expand All @@ -108,21 +88,15 @@ const config = {
'no-empty': 'error',
'no-eval': 'error',
'no-new-wrappers': 'error',
// handles only member sort, rest is handled via import/order
'sort-imports': ['warn', { ignoreDeclarationSort: true }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new rule - handles sorting members within import statements

'no-restricted-globals': [
'error',
...['blur', 'close', 'focus', 'length', 'name', 'parent', 'self', 'stop'].map(name => ({
name,
message: `"${name}" refers to a DOM global. Did you mean to reference a local value instead?`,
})),
],
'@fluentui/ban-imports': [
'error',
{
path: 'react',
names: ['useLayoutEffect'],
message: '`useLayoutEffect` causes a warning in SSR. Use `useIsomorphicLayoutEffect`',
},
],
'no-restricted-properties': [
'error',
{ object: 'describe', property: 'only', message: 'describe.only should only be used during test development' },
Expand All @@ -138,53 +112,6 @@ const config = {
'prefer-const': 'error',
'prefer-arrow-callback': 'error', // tslint: no-function-expression
radix: ['error', 'always'],
'react/jsx-key': 'error',
'react/jsx-no-bind': [
'error',
{
allowArrowFunctions: false, // tslint: jsx-no-lambda
allowFunctions: false,
allowBind: false,
ignoreDOMComponents: true,
ignoreRefs: true,
},
],
'react/no-string-refs': 'error',
'react/self-closing-comp': 'error',
'react-hooks/exhaustive-deps': [
'error',
{
additionalHooks: 'useIsomorphicLayoutEffect',
},
],
'react-hooks/rules-of-hooks': 'error',

// airbnb or other config overrides (some temporary)
// TODO: determine which rules we want to enable, and make needed changes (separate PR)
'arrow-body-style': 'off',
'class-methods-use-this': 'off',
'consistent-return': 'off',
'default-case': 'off',
'func-names': 'off',
'global-require': 'off',

'jsx-a11y/alt-text': 'off',
'jsx-a11y/anchor-is-valid': 'off',
'jsx-a11y/aria-activedescendant-has-tabindex': 'off',
'jsx-a11y/aria-role': 'off',
'jsx-a11y/click-events-have-key-events': 'off',
'jsx-a11y/control-has-associated-label': 'off',
'jsx-a11y/role-has-required-aria-props': 'off',
'jsx-a11y/interactive-supports-focus': 'off',
'jsx-a11y/label-has-associated-control': 'off',
'jsx-a11y/media-has-caption': 'off',
'jsx-a11y/mouse-events-have-key-events': 'off',
'jsx-a11y/no-interactive-element-to-noninteractive-role': 'off',
'jsx-a11y/no-noninteractive-element-interactions': 'off',
'jsx-a11y/no-noninteractive-tabindex': 'off',
'jsx-a11y/no-redundant-roles': 'off',
'jsx-a11y/no-static-element-interactions': 'off',
'jsx-a11y/role-supports-aria-props': 'off',
'lines-between-class-members': 'off',
'max-classes-per-file': 'off',
'no-case-declarations': 'off',
Expand Down Expand Up @@ -216,6 +143,93 @@ const config = {
'operator-assignment': 'off',
'prefer-destructuring': 'off',
'prefer-template': 'off',
// airbnb or other config overrides (some temporary)
// TODO: determine which rules we want to enable, and make needed changes (separate PR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to open a ticket for this right away?

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 dunno TBH. as stated in PR I didn't spent extra time to go through every rule rather colocating for better readability. Feel free to investigate further

Copy link
Member

@ecraig12345 ecraig12345 Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is from back when we originally migrated to eslint. The idea back then was to extend the airbnb config (maybe because v0 did?) and eventually enable more rules as we thought it made sense, but we never got around to following up. Since we're disabling so many airbnb rules, it would probably make more sense to just directly enable the rules from there that we do want and remove that config extension (in a separate PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this ^ 100%. I'd like us to remove airbnb completely (unmaintaned, loooots of opinionated rules that are not useful that much. I'll work on this as a follow up.

'arrow-body-style': 'off',
'class-methods-use-this': 'off',
'consistent-return': 'off',
'default-case': 'off',
'func-names': 'off',
'global-require': 'off',
'spaced-comment': 'off',
// airbnb options ban for-of which is unnecessary for TS and modern node (https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/style.js#L334)
// but this is a very powerful rule we may want to use in other ways
'no-restricted-syntax': 'off',
// permanently disable because we disagree with these rules
'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable
// permanently disable due to performance issues (using custom rule `@fluentui/max-len` instead)
'max-len': 'off',
// permanently disable due to perf problems and limited benefit
// see here for perf testing (note that you must run eslint directly)
// https://eslint.org/docs/developer-guide/working-with-rules#per-rule-performance
'no-empty-character-class': 'off',

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a picky unimportant thing but all the blank lines starting the comments are bothering me, and IMO it would look nicer to get rid of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you said, not relevant to this PR, we can address as follow up

* `@typescript-eslint`plugin eslint rules
* @see https://github.com/typescript-eslint/typescript-eslint/tree/main/packages/eslint-plugin
*/
// tslint: function-name, variable-name
...configHelpers.getNamingConventionRule(false),
'@typescript-eslint/no-empty-function': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@typescript-eslint/prefer-namespace-keyword': 'error',

/**
*
* `@rnx-kit` eslint rules
* @see https://github.com/microsoft/rnx-kit/tree/main/packages/eslint-plugin
*/
'@rnx-kit/no-export-all': ['error', { expand: 'external-only' }],

/**
*
* `@fluentui` eslint rules / This package
* @see https://www.npmjs.com/package/@fluentui/eslint-plugin
*/
'@fluentui/ban-imports': [
'error',
{
path: 'react',
names: ['useLayoutEffect'],
message: '`useLayoutEffect` causes a warning in SSR. Use `useIsomorphicLayoutEffect`',
},
],
'@fluentui/no-global-react': 'error',
'@fluentui/max-len': [
'error',
{
ignorePatterns: [
'require(<.*?>)?\\(',
'https?:\\/\\/',
'^(import|export) ',
'^\\s+(<path )?d=',
'!raw-loader',
'\\bdata:image/',
],
max: 120,
},
],
'@fluentui/no-tslint-comments': 'error',

/**
*
* react eslint rules
* @see https://github.com/yannickcr/eslint-plugin-react
*/
'react/jsx-key': 'error',
'react/jsx-no-bind': [
'error',
{
allowArrowFunctions: false, // tslint: jsx-no-lambda
allowFunctions: false,
allowBind: false,
ignoreDOMComponents: true,
ignoreRefs: true,
},
],
'react/no-string-refs': 'error',
'react/self-closing-comp': 'error',
'react/button-has-type': 'off',
'react/destructuring-assignment': 'off',
'react/forbid-prop-types': 'off',
Expand All @@ -235,30 +249,56 @@ const config = {
'react/state-in-constructor': 'off',
'react/static-property-placement': 'off',
'react/require-default-props': 'off',
'spaced-comment': 'off',

// airbnb options ban for-of which is unnecessary for TS and modern node (https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/style.js#L334)
// but this is a very powerful rule we may want to use in other ways
'no-restricted-syntax': 'off',

// permanently disable because we disagree with these rules
'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable
'react/jsx-props-no-spreading': 'off',
'react/prop-types': 'off',

// permanently disable due to performance issues (using custom rule `@fluentui/max-len` instead)
'max-len': 'off',

// permanently disable due to perf problems and limited benefit
// see here for perf testing (note that you must run eslint directly)
// https://eslint.org/docs/developer-guide/working-with-rules#per-rule-performance
'no-empty-character-class': 'off',
'react/no-unknown-property': 'off', // expensive, limited benefit with TS
// these ones have minor negative perf impact and are unnecessary
'react/default-props-match-prop-types': 'off',
'react/no-unused-prop-types': 'off',
'react/prefer-es6-class': 'off',
// permanently disable because we disagree with these rules
'react/jsx-props-no-spreading': 'off',
'react/prop-types': 'off',

/**
*
* react-hooks rules
* @see https://github.com/facebook/react/tree/main/packages/eslint-plugin-react-hooks
*/
'react-hooks/exhaustive-deps': [
'error',
{
additionalHooks: 'useIsomorphicLayoutEffect',
},
],
'react-hooks/rules-of-hooks': 'error',

/**
*
* jsx-a11y rules
* @see https://github.com/jsx-eslint/eslint-plugin-jsx-a11y
*/
'jsx-a11y/alt-text': 'off',
'jsx-a11y/anchor-is-valid': 'off',
'jsx-a11y/aria-activedescendant-has-tabindex': 'off',
'jsx-a11y/aria-role': 'off',
'jsx-a11y/click-events-have-key-events': 'off',
'jsx-a11y/control-has-associated-label': 'off',
'jsx-a11y/role-has-required-aria-props': 'off',
'jsx-a11y/interactive-supports-focus': 'off',
'jsx-a11y/label-has-associated-control': 'off',
'jsx-a11y/media-has-caption': 'off',
'jsx-a11y/mouse-events-have-key-events': 'off',
'jsx-a11y/no-interactive-element-to-noninteractive-role': 'off',
'jsx-a11y/no-noninteractive-element-interactions': 'off',
'jsx-a11y/no-noninteractive-tabindex': 'off',
'jsx-a11y/no-redundant-roles': 'off',
'jsx-a11y/no-static-element-interactions': 'off',
'jsx-a11y/role-supports-aria-props': 'off',

/**
*
* jsdoc plugin rules
* @see https://github.com/gajus/eslint-plugin-jsdoc
*/
'jsdoc/check-tag-names': [
'error',
{
Expand All @@ -273,15 +313,24 @@ const config = {
* @see https://github.com/import-js/eslint-plugin-import
*/
'import/no-extraneous-dependencies': ['error', { devDependencies: false }],
'import/no-duplicates': 'warn',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new rules

  • handles sorting imports by path
  • handles duplicate path imports
  • enforces to have imports always at the top of the file ( default behaviour of ECMAscript anyways - imports are hoisted )

Copy link
Member

@ecraig12345 ecraig12345 Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't the new rules going to cause a bunch of spam in build logs due to existing violations? (edit: it adds THOUSANDS of new warnings across all projects. 😱) And I still don't really see the point of even having rules if you can just ignore the warnings (especially since it creates a nuisance for others running lint because they see warnings from code they didn't even touch).

Copy link
Member

Choose a reason for hiding this comment

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

Agree on warnings. They add a lot of noise and not much help. I'd prefer autofix or failing rules only. Otherwise, it probably isn't really a rule if it can't be autofixed or if we can't force the dev to fix it.

'import/first': 'warn',
'import/order': [
'warn',
{
'newlines-between': 'always',
alphabetize: {
order: 'asc',
caseInsensitive: false,
},
},
],
'import/extensions': 'off',
'import/first': 'off',
'import/newline-after-import': 'off',
'import/no-duplicates': 'off', // mostly redundant with no-duplicate-imports
'import/no-dynamic-require': 'off',
'import/no-mutable-exports': 'off',
'import/no-unresolved': 'off',
'import/no-useless-path-segments': 'off',
'import/order': 'off',
'import/prefer-default-export': 'off',
// may cause perf problems per https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#eslint-plugin-import
'import/no-cycle': 'off',
Expand Down