[core] Migrate from tslint to eslint

[ZeeshanTamboli]

• I have followed (at least) the PR section of the contributing guide.

Closes #19413

Should also unblock #38780.

Tried using recommended rules with @typescript-eslint/recommended but it throws 2534 errors, so not using it.

[mui-bot]

Netlify deploy preview

https://deploy-preview-40020--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 34d004e

[Janpot]

Regarding the original issue: #19413 (comment)

Are these requirements solved? Or outdated?

[ZeeshanTamboli]

Regarding the original issue: #19413 (comment)

Are these requirements solved? Or outdated?

@Janpot I think those rules are still active in master.

Regarding dtslint/no-relative-import-from-test, for testing, I changed relative import for a spec file in master branch:

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

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]

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]

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:

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

My take on it would be:

1. 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.
2. 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]

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

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]

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]

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.