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.

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

[core] Lint with typescript-eslint parser #21758

Merged
merged 16 commits into from
Jul 16, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 11, 2020

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 core Infrastructure work going on behind the scenes labels Jul 11, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 11, 2020

Details of bundle changes

Generated by 🚫 dangerJS against d24c890

@eps1lon
Copy link
Member

eps1lon commented Jul 12, 2020

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
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 :).

extends: [
'plugin:import/recommended',
'plugin:import/typescript',
'airbnb-typescript',
Copy link
Member

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?

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 13, 2020

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.

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 13, 2020

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
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.

.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Member

@eps1lon eps1lon left a comment

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 👍

.eslintignore Outdated Show resolved Hide resolved
extends: [
'plugin:import/recommended',
'plugin:import/typescript',
'airbnb-typescript',
Copy link
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.

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated
rules: {
'@typescript-eslint/dot-notation': 'off', // Too slow
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 13, 2020

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.

.eslintrc.js Outdated Show resolved Hide resolved
packages/material-ui/src/Button/Button.spec.tsx Outdated Show resolved Hide resolved
packages/material-ui/src/index.d.ts Outdated Show resolved Hide resolved
packages/material-ui/src/styles/colorManipulator.js Outdated Show resolved Hide resolved
packages/material-ui/src/styles/transitions.test.js Outdated Show resolved Hide resolved
@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
@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
'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
Member

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
Member

Choose a reason for hiding this comment

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

If you don't care about restricting the file extensions that may contain JSX.

-- https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-filename-extension.md

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 16, 2020

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants