[core] Migrate from tslint to eslint #40020

--- a/packages/mui-material/src/Box/Box.spec.tsx
+++ b/packages/mui-material/src/Box/Box.spec.tsx
@@ -1,8 +1,8 @@
 import * as React from 'react';
 import { Box as SystemBox, createBox } from '@mui/system';
 import { expectType } from '@mui/types';
-import Box from '@mui/material/Box';
 import { createTheme } from '@mui/material/styles';
+import Box from './Box';

 function ThemeValuesCanBeSpread() {
   <Box

And after running:

cd packages/mui-material
yarn typescript

I do get the following error:

packages/mui-material/src/Box/Box.spec.tsx:5:17 - Test file should not use a relative import. Use a

 global import as if this were a user of the package. See: https://github.com/Microsoft/dtslint/blob/master/docs/no-relat

ive-import-in-test.md

So, in this migration branch, I attempted to shift this rule to ESLint using the no-restricted-imports rule:

    // no relative imports in TypeScript spec files
    {
      files: ['packages/*/{src,test}/**/*.spec.{ts,tsx}'],
      rules: {
        'no-restricted-imports': [
          'error',
          {
            patterns: ['.*'],
          },
        ],
      },
    }

However, this resulted in errors for other spec files:

$ yarn eslint
yarn run v1.22.19
$ eslint . --cache --report-unused-disable-directives --ext .js,.ts,.tsx --max-warnings 0

F:\material-ui\packages\mui-base\src\FormControl\FormControl.spec.tsx
  4:1  error  './FormControl.types' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-base\src\TablePagination\TablePagination.spec.tsx
  4:1  error  './TablePagination.types' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-base\src\utils\appendOwnerState.spec.tsx
  2:1  error  './appendOwnerState' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-base\src\utils\prepareForSlot.spec.tsx
  2:1  error  './prepareForSlot' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-material-next\src\FormHelperText\FormHelperText.spec.tsx
  4:1  error  './FormHelperText.types' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-material-next\src\Slider\Slider.spec.tsx
  3:1  error  './Slider.types' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-utils\src\chainPropTypes\chainPropTypes.spec.tsx
  3:1  error  './chainPropTypes' import is restricted from being used by a pattern  no-restricted-imports

F:\material-ui\packages\mui-utils\src\setRef.spec.tsx
  2:1  error  './setRef' import is restricted from being used by a pattern  no-restricted-imports

✖ 8 problems (8 errors, 0 warnings)

How can we solve this? I'm wondering if there's a ts configuration option to detect this instead of relying on ESLint.

As for the deprecation rule, I have not tested it, but the equivalent ESLint rule is deprecation/deprecation. The TSLint deprecation rule warns instead of causing an error. Do you think it's worthwhile to set up the ESLint deprecation plugin to issue warnings?

Additionally, considering the discovery of other TSLint rules that seem to be active by default, such as deprecation and dtslint/no-relative-import-in-test, it might be that other rules are also active by default.

I would not have been able to find them without it being explicitly set in the tslint.json config.

Also, I referred to this list.

Edit: I discovered that all rules marked as default (those that are not specified in tslint.json) are set to error severity due to the top-level defaultSeverity: error setting here. Can you confirm whether I should proceed to add the corresponding alternate ESLint rules for these?

michaldudak · 2023-12-01T09:39:57Z

However, this resulted in errors for other spec files

Looks like the linter is correct - these imports shouldn't be relative according to our rules.

However, I don't really see a point in enforcing package-style imports, as paths are rewritten in tests anyway to point to local files. Does this bring any value other than enforcing a convention?

Can you confirm whether I should proceed to add the corresponding alternate ESLint rules for these?

If they are available, sure. One thing to note is that some ESLint rules may be very slow - for example the deprecation rule you mentioned is unusable (I tried it in #38087)

Additionally, considering the discovery of other TSLint rules that seem to be active by default, such as deprecation and dtslint/no-relative-import-in-test, it might be that other rules are also active by default.

IMO we don't need to have a perfect 1-1 migration between these linters. If we ever spot a pattern that should be discouraged, we can update the config. Let's try to do most of them if possible, but I don't think it's worth spending days on figuring out all the TSLint rules equivalents.

Janpot · 2023-12-01T13:41:16Z

IMO we don't need to have a perfect 1-1 migration between these linters.

I agree, that's also not my intention. But we need to make sure we don't drop useful rules. And if we drop them in the context of a certain trade-off, it could be interesting to document that trade-off in the PR.

ZeeshanTamboli · 2023-12-05T06:29:27Z

Does this bring any value other than enforcing a convention?

I am not sure. Might be just a convention.

Sure, I'll check out some helpful rules.

Also note that tslint was enabled only for spec files (spec.ts/tsx) and declaration files (.d.ts) in packages.

Janpot

Ok, so to understand the spirit of why this linter was enabled was to:

Make sure we are not testing deprecated behavior
Make sure we import the library by bare specifier in tests.

My take on it would be:

When new features are deprecated, we should write new tests and keep the old ones. Then remove the old tests wwhen the old behavior is removed. I'm not sure the deprecated rule is very helpful in this process.
I only find this useful if it means we also rely on default module resolution, but the way it's currently set up is that the bare specifier is an alias for the relative module. I don't see how it adds much value, first thing the bundler does is replace the bare specifier with the relative one.

Ok, we can move forward with this 👍

oliviertassinari · 2023-12-05T12:30:54Z

Make sure we import the library by bare specifier in tests.

We had cases in the past where tests would depend on private API. By importing from the barrel index, we force that tests to not rely on private API (even if it doesn't use the exact same code path as developer experiences, the current aliasing system is good enough for that purpose).

In any case, I won't miss dtslint/no-relative-import-in-test, we have 'no-restricted-imports' rules in eslint that covers the same problem. Well, we will need to tweak this rule and fix all the violations but we can have the same coverage.

Janpot · 2023-12-05T12:42:59Z

We had cases in the past where tests would depend on private API. By importing from the barrel index, we force that tests to not rely on private API

I like that. One solution could be to move the test folder outside of the package folder and into its own workspace (or, I guess, all tests across packages gathered in one workspace). Another advantage of that is that this setup brings us closer to being able to test the actual built package. One drawback would be that tests are less colocated with the implementation.

oliviertassinari · 2023-12-05T13:13:52Z

But I'm not aware we had issues with the current import setup, the import logic seems to have been close enough so far.

Janpot · 2023-12-05T16:12:08Z

But I'm not aware we had issues with the current import setup, the import logic seems to have been close enough so far.

We're unable to replicate the current tslint behavior around these relative imports with eslint-plugin-import. I might be missing something though. I believe we can rely on no-relative-packages if the tests were in their own workspace.

Besides that, we have regularly issues with our current setup, e.g. importing the zero config runtime in the docs, or internal packages in X, or in the setup up of most static analysis tools. but that's out of scope for this PR.

initial changes

8fee886

ZeeshanTamboli added typescript core Infrastructure work going on behind the scenes labels Nov 27, 2023

ZeeshanTamboli added 16 commits November 27, 2023 17:52

changes

c79a829

yarn deduplicate

64b9d1c

fix syntax

fc9cb29

remove source files from command

f502211

remove path

d5c6f9f

remove unnecessary tsconfig file

993f502

extend root tsconfig

307d1f1

fix tsconfig

914c057

fix tsconfig for icons typing test

1421f9a

prettier

ccdf691

testing removing disable max-line-length rule of tslint

c5dcc9a

remove all max-line-length instances

0130b55

remove tslint rules

ee1c0af

run tsc for icon-material package

f89e795

remove unnecessary test folder includes in tsconfigs

0c12d8b

Merge branch 'master' into migrate-tslint-to-eslint

59e685f

ZeeshanTamboli changed the title ~~[WIP] Migrate tslint to eslint~~ [core] Migrate tslint to eslint Nov 28, 2023

ZeeshanTamboli marked this pull request as ready for review November 28, 2023 12:27

ZeeshanTamboli requested a review from a team November 28, 2023 12:30

Janpot reviewed Nov 28, 2023

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Janpot reviewed Nov 28, 2023

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Janpot reviewed Nov 28, 2023

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

ZeeshanTamboli added 2 commits November 29, 2023 12:23

remove unnecessary stylistic rules already handled by prettier

5fdbe43

remove stylistic eslint plugin js

549a487

ZeeshanTamboli requested a review from Janpot November 29, 2023 07:15

ZeeshanTamboli changed the title ~~[core] Migrate tslint to eslint~~ [core] Migrate from tslint to eslint Nov 29, 2023

oliviertassinari added the scope: code-infra Specific to the core-infra product label Dec 2, 2023

Merge branch 'master' into migrate-tslint-to-eslint

5466dc7

Janpot approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'master' into migrate-tslint-to-eslint

34d004e

ZeeshanTamboli merged commit ab9445e into mui:master Dec 5, 2023
22 checks passed

ZeeshanTamboli deleted the migrate-tslint-to-eslint branch December 5, 2023 12:04

ZeeshanTamboli mentioned this pull request Dec 5, 2023

Bump typescript to v5.3.2 #38780

Merged

1 task

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023

[core] Migrate from tslint to eslint (mui#40020)

28e1c38

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023

[core] Migrate from tslint to eslint (mui#40020)

dd54047

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Dec 6, 2023

[core] Migrate from tslint to eslint (mui#40020)

1476d2f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Migrate from tslint to eslint #40020

[core] Migrate from tslint to eslint #40020

ZeeshanTamboli commented Nov 27, 2023 •

edited

mui-bot commented Nov 27, 2023 •

edited

Janpot commented Nov 29, 2023

ZeeshanTamboli commented Nov 29, 2023 •

edited

michaldudak commented Dec 1, 2023

Janpot commented Dec 1, 2023 •

edited

ZeeshanTamboli commented Dec 5, 2023 •

edited

Janpot left a comment

oliviertassinari commented Dec 5, 2023 •

edited

Janpot commented Dec 5, 2023 •

edited

oliviertassinari commented Dec 5, 2023 •

edited

Janpot commented Dec 5, 2023

[core] Migrate from tslint to eslint #40020

[core] Migrate from tslint to eslint #40020

Conversation

ZeeshanTamboli commented Nov 27, 2023 • edited

mui-bot commented Nov 27, 2023 • edited

Netlify deploy preview

Bundle size report

Janpot commented Nov 29, 2023

ZeeshanTamboli commented Nov 29, 2023 • edited

michaldudak commented Dec 1, 2023

Janpot commented Dec 1, 2023 • edited

ZeeshanTamboli commented Dec 5, 2023 • edited

Janpot left a comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 5, 2023 • edited

Janpot commented Dec 5, 2023 • edited

oliviertassinari commented Dec 5, 2023 • edited

Janpot commented Dec 5, 2023

ZeeshanTamboli commented Nov 27, 2023 •

edited

mui-bot commented Nov 27, 2023 •

edited

ZeeshanTamboli commented Nov 29, 2023 •

edited

Janpot commented Dec 1, 2023 •

edited

ZeeshanTamboli commented Dec 5, 2023 •

edited

oliviertassinari commented Dec 5, 2023 •

edited

Janpot commented Dec 5, 2023 •

edited

oliviertassinari commented Dec 5, 2023 •

edited