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-infra] Add no-relative-packages #44489

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 20, 2024

Move mui/mui-x#15437 from MUI X to code-infra.

This breaks in Base UI. Some are nice, they show cases where we import private APIs in the app/experiments some are weird, e.g. https://github.com/mui/base-ui/blob/d7809157ccc737608acc70a1ecbb42dfbca7ae78/packages/mui-base/vitest.config.ts#L2 seems fine. So I guess it makes sense to move forward.

@mui-bot
Copy link

mui-bot commented Nov 20, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against fa8ffd2

@Janpot
Copy link
Member

Janpot commented Nov 21, 2024

It seems fine.

it mostly is, the main problem I've seen so far is that it stops us from being able to rely on workspace topology to detect changes. e g. to run tests selectively. for things like the vitest config, we could always place those in their own package.

@@ -4,6 +4,7 @@ import fetch from 'cross-fetch';
import fse from 'fs-extra';
import * as mui from '@mui/icons-material';
import synonyms from 'docs/data/material/components/material-icons/synonyms';
// eslint-disable-next-line import/no-relative-packages
import myDestRewriter from '../../packages/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.

can likely be reorganized to live under this package instead. but low priority.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 22, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 23, 2024
Comment on lines -245 to -246
// Some of these occurences are deliberate and fixing them will break things in repos that use @monorepo dependency
'import/no-relative-packages': 'off',
Copy link
Member Author

Choose a reason for hiding this comment

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

The change of this PR

@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label Nov 23, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 25, 2024

being able to rely on workspace topology to detect changes. e g. to run tests selectively. for things like the vitest config, we could always place those in their own package.

Oh, interesting, I would have assumed that change detections would resolve all imports, relative or absolute.

In any cases, this PR seems safe to go with. I don't really like that it make it harder to import stuff relative, but I sense that the general desire of the team is to get more package isolation inside repositories, this helps.

@oliviertassinari oliviertassinari merged commit ef68514 into mui:master Nov 25, 2024
22 checks passed
@oliviertassinari oliviertassinari deleted the no-relative-packages branch November 25, 2024 14:06
@Janpot
Copy link
Member

Janpot commented Nov 27, 2024

Oh, interesting, I would have assumed that change detections would resolve all imports, relative or absolute.

I assume change detection algorithms mostly work by following workspace dependencies and not so much by evaluating each and every potential import in each and every file in a package. I expect a change detection algorithm to only work reliably if all the package boundaries are respected religiously.

@LukasTy LukasTy mentioned this pull request Nov 29, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants