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] Babel plugin to fully resolve imported paths #43294

Merged
merged 29 commits into from
Aug 22, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 14, 2024

Working on #43264 I found that we can make rollup work quite well for us to resolve modules, except that they inline interop helpers that we can't seem to dedupe the way we can in babel.

This PR solves the problem differently with a custom babel plugin that can resolve module imports to their actual emitted file.

This PR changes imports in the build output from

// packages/mui-material/build/index.js
export * from './Accordion';

// packages/mui-material/build/Breadcrumbs/BreadcrumbCollapsed.js
import MoreHorizIcon from '../internal/svg-icons/MoreHoriz';

to

// packages/mui-material/build/index.js
export * from './Accordion/index.js';

// packages/mui-material/build/Breadcrumbs/BreadcrumbCollapsed.js
import MoreHorizIcon from '../internal/svg-icons/MoreHoriz.js';

It resolves those imports to the full filename with extension.

See https://nodejs.org/docs/v20.16.0/api/esm.html#mandatory-file-extensions

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.

This behavior matches how import behaves in browser environments, assuming a typically configured server.

This doesn't update imports for the icons package, as these modules are pre-baked in the repository. We can run pnpm --filter @mui/icons-material build:lib to update the icons as part of this PR or separately.

I'm also adding some type checking to the babel.config.js

Corresponding PR on X


Noticed this problem while testing out the changes.

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Aug 14, 2024
@mui-bot
Copy link

mui-bot commented Aug 14, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 86fae26

@Janpot Janpot changed the title [code-infra] Babel plugin to fully resolve imported paths [code-infra] Babel plugin to fully resolve imported paths (WIP/experiment) Aug 14, 2024
@Janpot Janpot changed the title [code-infra] Babel plugin to fully resolve imported paths (WIP/experiment) [code-infra] Babel plugin to fully resolve imported paths (WIP) Aug 16, 2024
@Janpot Janpot requested a review from DiegoAndai August 19, 2024 06:12
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 19, 2024
@Janpot Janpot changed the title [code-infra] Babel plugin to fully resolve imported paths (WIP) [code-infra] Babel plugin to fully resolve imported paths Aug 19, 2024
@Janpot Janpot marked this pull request as ready for review August 19, 2024 15:24
@Janpot
Copy link
Member Author

Janpot commented Aug 19, 2024

Have been testing this locally from csbci published packages and can't seem to break it

@Janpot Janpot requested a review from a team August 19, 2024 15:25
@michaldudak
Copy link
Member

The implementation looks good, but could you place the plugin somewhere we can package and distribute with npm (internal-scripts, perhaps)?

@Janpot
Copy link
Member Author

Janpot commented Aug 21, 2024

Done. Added a @mui/internal-babel-plugin-resolve-imports package

edit: added it to csbci as well

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,30 @@
{
"name": "@mui/internal-babel-plugin-resolve-imports",
"version": "1.0.16",
Copy link
Member

Choose a reason for hiding this comment

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

That's unusual initial version ;)

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 copied the package.json of @mui/internal-scripts and didn't give it any further thought.

@Janpot Janpot merged commit f4a7047 into mui:next Aug 22, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants