[core] Lint with typescript-eslint parser #21758
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| const path = require('path'); | ||
|
|
||
| module.exports = { | ||
|
|
@@ -11,13 +10,19 @@ module.exports = { | |
| browser: true, | ||
| node: true, | ||
| }, | ||
| extends: [ | ||
| 'plugin:import/recommended', | ||
| 'plugin:import/typescript', | ||
| 'airbnb-typescript', | ||
| 'prettier', | ||
| 'prettier/react', | ||
| 'prettier/@typescript-eslint', | ||
| ], | ||
| parser: '@typescript-eslint/parser', | ||
| parserOptions: { | ||
| ecmaVersion: 7, | ||
eps1lon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| plugins: ['material-ui', 'react-hooks', '@typescript-eslint'], | ||
| settings: { | ||
| 'import/resolver': { | ||
| webpack: { | ||
|
|
@@ -31,16 +36,11 @@ module.exports = { | |
| */ | ||
| rules: { | ||
| 'consistent-this': ['error', 'self'], | ||
| // just as bad as "max components per file" | ||
| 'max-classes-per-file': 'off', | ||
| 'no-alert': 'error', // Too much interruptive | ||
| 'no-console': ['error', { allow: ['warn', 'error'] }], // Allow warn and error for production events | ||
| 'no-param-reassign': 'off', // It's fine. | ||
| 'no-restricted-imports': [ | ||
| 'error', | ||
| { | ||
|
|
@@ -51,64 +51,38 @@ module.exports = { | |
| ], | ||
| }, | ||
| ], | ||
eps1lon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 'no-constant-condition': 'error', | ||
| 'no-prototype-builtins': 'off', // Use the proptype inheritance chain | ||
|
There was a problem hiding this comment. Do you agree with the rule? https://eslint.org/docs/rules/no-prototype-builtins. If we don't disable it, it fails in 5 places, for instance in the Modal
We would need to fix it with the proposed approach to the eslint documentation. It seems to be about hedging against global prototype pollution. Is it necessary? There was a problem hiding this comment. I'd rather disable these on a per line basis. I do follow the rationale but this rule also documents very well when it should be turned of. We can't guarantee it globally though. E.g. |
||
| 'no-underscore-dangle': 'error', | ||
| 'nonblock-statement-body-position': 'error', | ||
| 'prefer-arrow-callback': ['error', { allowNamedFunctions: true }], | ||
| 'prefer-destructuring': 'off', // Destructuring harm grep potential. | ||
| '@typescript-eslint/dot-notation': 'off', // TODO performance consideration | ||
| '@typescript-eslint/no-implied-eval': 'off', // TODO performance consideration | ||
| '@typescript-eslint/no-throw-literal': 'off', // TODO performance consideration | ||
| 'import/named': 'off', // Not sure why it doesn't work | ||
| 'import/no-extraneous-dependencies': 'off', // Missing yarn workspace support | ||
| 'jsx-a11y/label-has-associated-control': 'off', // doesn't work? | ||
| 'jsx-a11y/no-autofocus': 'off', // We are a library, we need to support it too | ||
| 'material-ui/docgen-ignore-before-comment': 'error', | ||
| 'react-hooks/exhaustive-deps': ['error', { additionalHooks: 'useEnhancedEffect' }], | ||
| 'react-hooks/rules-of-hooks': 'error', | ||
| 'react/destructuring-assignment': 'off', // It's fine. | ||
| 'react/forbid-prop-types': 'off', // Too strict, no time for that | ||
| 'react/jsx-curly-brace-presence': 'off', // broken | ||
| 'react/jsx-filename-extension': ['error', { extensions: ['.js', '.tsx'] }], // airbnb is using .jsx | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment.
|
||
| 'react/jsx-fragments': ['error', 'element'], // Prefer <React.Fragment> over <>. | ||
| 'react/jsx-props-no-spreading': 'off', // We are a UI library. | ||
| 'react/no-array-index-key': 'off', // This rule is great for raising people awareness of what a key is and how it works. | ||
| 'react/no-danger': 'error', | ||
| 'react/no-direct-mutation-state': 'error', | ||
| 'react/no-find-dom-node': 'off', // Required for backward compatibility. TODO v5, drop | ||
| 'react/require-default-props': 'off', // Not always relevant | ||
| 'react/sort-prop-types': 'error', | ||
| // This depends entirely on what you're doing. There's no universal pattern | ||
| 'react/state-in-constructor': 'off', | ||
| // stylistic opinion. For conditional assignment we want it outside, otherwise as static | ||
| 'react/static-property-placement': 'off', | ||
| }, | ||
| overrides: [ | ||
| { | ||
|
|
@@ -124,7 +98,6 @@ module.exports = { | |
| rules: { | ||
| // does not work with wildcard imports. Mistakes will throw at runtime anyway | ||
| 'import/named': 'off', | ||
| 'no-restricted-imports': [ | ||
| 'error', | ||
| { | ||
|
|
@@ -191,5 +164,53 @@ module.exports = { | |
| 'react/prop-types': 'off', | ||
| }, | ||
| }, | ||
| { | ||
| files: ['*.d.ts'], | ||
| rules: { | ||
| 'import/export': 'off', // Not sure why it doesn't work | ||
| }, | ||
| }, | ||
| { | ||
| files: ['*.tsx'], | ||
| rules: { | ||
| 'no-restricted-imports': [ | ||
|
There was a problem hiding this comment. This looks duplicated from the base rules. There was a problem hiding this comment. It allows an extra level of depth import compared to the base rule. It solves the Breakpoint problem I solved previously with duplicating the types in the demos (which I revered) There was a problem hiding this comment. Yeah but that isn't correct. We should only be testing public types. If we don't have a public type for breakpoints then this is a problem. There was a problem hiding this comment. Agree, we need to fix that missing public export |
||
| 'error', | ||
| { | ||
| patterns: [ | ||
| '@material-ui/*/*/*/*', | ||
| '!@material-ui/core/test-utils/*', | ||
| '!@material-ui/utils/macros/*.macro', | ||
| ], | ||
| }, | ||
| ], // Allow deeper imports for TypeScript types. TODO? | ||
| 'react/prop-types': 'off', | ||
| }, | ||
| }, | ||
| { | ||
| files: ['*.spec.tsx', '*.spec.ts'], | ||
| rules: { | ||
| 'no-alert': 'off', | ||
| 'no-console': 'off', | ||
| 'no-empty-pattern': 'off', | ||
| 'no-lone-blocks': 'off', | ||
| 'no-shadow': 'off', | ||
| '@typescript-eslint/no-unused-expressions': 'off', | ||
| '@typescript-eslint/no-unused-vars': 'off', | ||
| '@typescript-eslint/no-use-before-define': 'off', | ||
| 'import/export': 'off', // Not sure why it doesn't work | ||
| 'import/prefer-default-export': 'off', | ||
| 'jsx-a11y/anchor-has-content': 'off', | ||
oliviertassinari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 'jsx-a11y/anchor-is-valid': 'off', | ||
| 'jsx-a11y/tabindex-no-positive': 'off', | ||
| 'react/default-props-match-prop-types': 'off', | ||
| 'react/no-access-state-in-setstate': 'off', | ||
| 'react/no-unused-prop-types': 'off', | ||
| 'react/prefer-stateless-function': 'off', | ||
| 'react/prop-types': 'off', | ||
| 'react/require-default-props': 'off', | ||
| 'react/state-in-constructor': 'off', | ||
| 'react/static-property-placement': 'off', | ||
| }, | ||
| }, | ||
| ], | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |
| import * as babel from '@babel/core'; | ||
| import traverse from '@babel/traverse'; | ||
| import { mkdirSync, readFileSync, writeFileSync } from 'fs'; | ||
| import { rewriteUrlForNextExport } from 'next/dist/next-server/lib/router/rewrite-url-for-export'; | ||
| import path from 'path'; | ||
| import kebabCase from 'lodash/kebabCase'; | ||
|
|
@@ -11,6 +10,7 @@ import { defaultHandlers, parse as docgenParse } from 'react-docgen'; | |
| import remark from 'remark'; | ||
| import remarkVisit from 'unist-util-visit'; | ||
| import * as yargs from 'yargs'; | ||
| import { getLineFeed } from './helpers'; | ||
|
There was a problem hiding this comment. nit: revert this re-arrangment for proper git history. There was a problem hiding this comment. This is required for one of the rules of the Airbnb preset There was a problem hiding this comment. Can we discuss introduction of new rules in another PR? There was a problem hiding this comment. Are you against the rule or the diff itself? There was a problem hiding this comment. If you are against the rule, happy to discuss it and revert if needed. If you are against the diff, I think that we will have to pay it at some time anyway, you & I already paid the cost now. I'm not sure it's worth spending time on reversing it. There was a problem hiding this comment. On this matter, we can also bring @dmtrKovalenko's rule to the table: https://github.com/dmtrKovalenko/eslint-plugin-pretty-imports. One ownside with it is that it requires to run eslint with the There was a problem hiding this comment.
What is the point of applying an order that is not reasonable to humans? I don't understand what ordering accomplishes here. There was a problem hiding this comment. Regarding ordering import, I think that the current rule communicates the broadness of the dependencies. broadness as the number of dependents. The first dependency being the one with the highest number of dependents: React. But in practice, it only split into two groups: global and relative imports. Why? Is it important? There was a problem hiding this comment.
I thought you didn't know how it is sorted? There was a problem hiding this comment. I'm advocating against eslint-plugin-pretty-imports (increase git diff, require running eslint --fix) but in favor of Airbnb default rule (enforce a split by global vs local imports, philosophy is to order by dependents). |
||
| import muiDefaultPropsHandler from '../src/modules/utils/defaultPropsHandler'; | ||
| import generateMarkdown from '../src/modules/utils/generateMarkdown'; | ||
| import { findPagesMarkdown, findComponents } from '../src/modules/utils/find'; | ||
|
|
||
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.
What the motivation of usage predefined config? Why not to use recommended by
@tpescript-eslint?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.
One of the end goal is to avoid developers or have to format the code once they copy and paste it from the demos. So the question we should answer to is: what's more popular in the community? Also, we have to consider that the stricter rules also cover lesser rules, meaning we can cover more developers by being more strict than the average.
The second goal is to make us more productive.
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.
Note that we were already using the Airbnb configuration. My objective was to lint TypeScript minimizing the diff. If we want to change the rules, that would be best in a different pull request.
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.
We can discuss the preset in another issue. This PR should be limited to switching the parser and adding TS files.