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

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

Merged

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Nov 15, 2024

This change should allow us to prevent issues like this: 0baadae

import { SizeProvider } from '../../../../x-charts/src/context/SizeProvider';

@JCQuintas JCQuintas added the scope: code-infra Specific to the core-infra product label Nov 15, 2024
@JCQuintas JCQuintas requested a review from a team November 15, 2024 19:17
@JCQuintas JCQuintas self-assigned this Nov 15, 2024
@JCQuintas JCQuintas enabled auto-merge (squash) November 15, 2024 19:17
@mui-bot
Copy link

mui-bot commented Nov 15, 2024

Deploy preview: https://deploy-preview-15437--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 2bcbfbf

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

It's strange that this was even needed. 🤔
Which IDE are you running?
I'm almost sure that I haven't seen VSCode suggesting cross-package imports. 🤔

The change makes sense, but my concern is the added runtime cost if not strictly needed. 🙈

@JCQuintas
Copy link
Member Author

I run vscode too...

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes and removed scope: code-infra Specific to the core-infra product labels Nov 15, 2024
@Janpot
Copy link
Member

Janpot commented Nov 16, 2024

Would it be possible to use the no-relative-packages rule here? And would it make sense to move this in the core repo instead?

@JCQuintas
Copy link
Member Author

Would it be possible to use the no-relative-packages rule here? And would it make sense to move this in the core repo instead?

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

but using no-relative-packages works for us 👍

@JCQuintas JCQuintas merged commit 33ca6ce into mui:master Nov 16, 2024
18 checks passed
@JCQuintas JCQuintas deleted the prevent-relative-imports-across-packges branch November 16, 2024 13:18
@@ -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

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

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.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants