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.

Sign up for GitHub

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

[code-infra] Prevent relative imports across packages #15437

Merged
merged 4 commits into from
Nov 16, 2024
Merged

[code-infra] Prevent relative imports across packages #15437

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c28bbe
[eslint] Add restriction for relative imports from other packages
JCQuintas Nov 15, 2024
92b910d
revert
JCQuintas Nov 15, 2024
1f53a10
Merge branch 'master' into prevent-relative-imports-across-packges
JCQuintas Nov 15, 2024
2bcbfbf
use no-relative-packages instead
JCQuintas Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ module.exports = {
...(ENABLE_REACT_COMPILER_PLUGIN ? { 'react-compiler/react-compiler': 'error' } : {}),
// TODO move to @mui/monorepo, codebase is moving away from default exports https://github.com/mui/material-ui/issues/21862
'import/prefer-default-export': 'off',
'import/no-relative-packages': 'error',
Copy link
Member

@oliviertassinari oliviertassinari Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

core can't enable this 🫠 https://github.com/mui/material-ui/blob/a0ffee42815b110e14107249f193b7505d1761e5/.eslintrc.js#L245-L246

This was initially added in mui/material-ui#34642. When I run the script again, I get only a few hits:

/Users/oliviertassinari/material-ui/apps/pigment-css-next-app/*

/Users/oliviertassinari/material-ui/apps/pigment-css-vite-app/*

/Users/oliviertassinari/material-ui/dangerFileContent.ts
  3:32  error  Relative import from another package is not allowed. Use `size-snapshot` instead of `./scripts/sizeSnapshot`                                                          import/no-relative-packages
  4:24  error  Relative import from another package is not allowed. Use `@mui-internal/api-docs-builder/utils/replaceUrl` instead of `./packages/api-docs-builder/utils/replaceUrl`  import/no-relative-packages

/Users/oliviertassinari/material-ui/docs/scripts/updateIconSynonyms.js
  7:28  error  Relative import from another package is not allowed. Use `@mui/icons-material/renameFilters/material-design-icons` instead of `../../packages/mui-icons-material/renameFilters/material-design-icons`  import/no-relative-packages

/Users/oliviertassinari/material-ui/packages/rsc-builder/buildRsc.ts
  4:28  error  Relative import from another package is not allowed. Use `@mui-internal/api-docs-builder/utils/findComponents` instead of `../api-docs-builder/utils/findComponents`  import/no-relative-packages
  5:23  error  Relative import from another package is not allowed. Use `@mui-internal/api-docs-builder/utils/findHooks` instead of `../api-docs-builder/utils/findHooks`            import/no-relative-packages

So I think the right move here is

Suggested change
'import/no-relative-packages': 'error',
// TODO move to @mui/monorepo
'import/no-relative-packages': 'error',
  1. Enabling the rule in the mono repo and opt-in out in the few places where we do this wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Which command did you run? I get

✖ 648 problems (648 errors, 0 warnings)
  648 errors and 0 warnings potentially fixable with the `--fix` option.

When changing

-'import/no-relative-packages': 'off',
+'import/no-relative-packages': 'error',

and running pnpm eslint:ci

Copy link
Member

@oliviertassinari oliviertassinari Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@JCQuintas I had the same number of errors: 648. I collapsed all the errors that seemed to be duplicated. So a "few" that needs to be fixed manually 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried it out and I don't think I have the necessary knowledge to make the right call on the material package changes 🙃

Once the changes are fixed some other issues appear:

/Users/jcquintas/dev/mui/material-ui/dangerFileContent.ts

// Can't resolve
import { loadComparison } from 'size-snapshot';

/Users/jcquintas/dev/mui/material-ui/docs/scripts/updateIconSynonyms.js

// eslint

  7:1   error  '@mui/icons-material/renameFilters/material-design-icons' import is restricted from being used by a pattern. Prefer one level nested imports to avoid bundling everything in dev mode or breaking CJS/ESM split.
See https://github.com/mui/material-ui/pull/24147 for the kind of win it can unlock  no-restricted-imports
  7:28  error  Unable to resolve path to module '@mui/icons-material/renameFilters/material-design-icons'

Copy link
Member

Choose a reason for hiding this comment

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

I tried in mui/material-ui#44489.

'import/no-restricted-paths': [
'error',
{
Expand Down