Skip to content

[internal] Lint with typescript-eslint parser#21758

Merged
oliviertassinari merged 16 commits into
mui:nextfrom
oliviertassinari:lint-typescript
Jul 16, 2020
Merged

[internal] Lint with typescript-eslint parser#21758
oliviertassinari merged 16 commits into
mui:nextfrom
oliviertassinari:lint-typescript

Conversation

@oliviertassinari

@oliviertassinari oliviertassinari commented Jul 11, 2020

Copy link
Copy Markdown
Member

This effort is part of a unification of the infrastructure that supports the authoring of outstanding React components for Material-UI between the main repository, the enterprise repository (written in TypeScript) and the pickers repository (written in TypeScript) that will likely be split between the main one for the MIT components and the x one for the enterprise features (#19706).

The objective is to find a configuration that works in the environment that sets the highest constraints (here) to then be able to copy and paste (until we find something better) the configuration to:

@oliviertassinari oliviertassinari added typescript internal Behind-the-scenes enhancement. Formerly called “core”. labels Jul 11, 2020
@mui-pr-bot

mui-pr-bot commented Jul 11, 2020

Copy link
Copy Markdown

Details of bundle changes

Generated by 🚫 dangerJS against d24c890

@eps1lon

eps1lon commented Jul 12, 2020

Copy link
Copy Markdown
Member

Heads up: We recently bumped eslint from 6 to 7 so this branch should be updated before considering to make this ready for review.

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 12, 2020
@oliviertassinari oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 12, 2020
@oliviertassinari
oliviertassinari marked this pull request as ready for review July 12, 2020 11:27
@oliviertassinari

Copy link
Copy Markdown
Member Author

Heads up: We recently bumped eslint from 6 to 7 so this branch should be updated

I have rebased, no change in the configuration was required :).

Comment thread .eslintrc.js
extends: [
'plugin:import/recommended',
'plugin:import/typescript',
'airbnb-typescript',

Copy link
Copy Markdown
Collaborator

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?

@oliviertassinari oliviertassinari Jul 13, 2020

Copy link
Copy Markdown
Member Author

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.

@oliviertassinari oliviertassinari Jul 13, 2020

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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.

Comment thread .eslintrc.js Outdated

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might've repeated myself but there's a lot to go through here. Big thanks to working on this 👍

Comment thread .eslintignore Outdated
Comment thread .eslintrc.js
extends: [
'plugin:import/recommended',
'plugin:import/typescript',
'airbnb-typescript',

Copy link
Copy Markdown
Member

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.

Comment thread .eslintrc.js Outdated
Comment thread .eslintrc.js Outdated
*/
// Sorted alphanumerically
rules: {
'@typescript-eslint/dot-notation': 'off', // Too slow

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

@oliviertassinari oliviertassinari Jul 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you enable the rules that are disabled with // Too slow, you will timeout the CI. No answers after 10 minutes. It seems to get stuck. I haven't looked at why.

Comment thread .eslintrc.js Outdated
Comment thread packages/material-ui/src/Button/Button.spec.tsx Outdated
Comment thread packages/material-ui/src/index.d.ts Outdated
Comment thread packages/material-ui/src/styles/colorManipulator.js Outdated
Comment thread packages/material-ui/src/styles/transitions.test.js Outdated
Comment thread packages/material-ui/test/typescript/components.spec.tsx
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 14, 2020
Comment thread docs/src/modules/components/TopLayoutBlog.js Outdated
@oliviertassinari oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 14, 2020
@oliviertassinari
oliviertassinari dismissed eps1lon’s stale review July 14, 2020 16:51

I have tried to take as many feedback as possible into account

@oliviertassinari oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 15, 2020
Comment thread .eslintrc.js
'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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why no .jsx here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari Jul 16, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

.jsx isn't allowed, influenced by airbnb/javascript#985 (comment)

@oliviertassinari
oliviertassinari merged commit ee6f54e into mui:next Jul 16, 2020
@oliviertassinari
oliviertassinari deleted the lint-typescript branch July 16, 2020 10:11
oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Jul 25, 2020
@eps1lon eps1lon mentioned this pull request Jul 28, 2020
@oliviertassinari oliviertassinari mentioned this pull request Aug 2, 2020
2 tasks
@oliviertassinari oliviertassinari changed the title [core] Lint with typescript-eslint parser [internal] Lint with typescript-eslint parser Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”. typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants