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] Remove commonjs imports in docs #44976

Merged
merged 26 commits into from
Jan 21, 2025
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jan 8, 2025

Tried to port regression test fixtures to vite but it chokes on these imports of commonjs modules. I've struggled several times as well with this in the past.

Removed the mixing of module systems altogether so no-one has to waste any more time on this. This should also unblock #44964

  • move to next.config.ts
  • docs/config: move from cjs to esm
  • @mui/internal-markdown: move from cjs to esm
  • split reportBrokenLinks into lib part and cli part. Avoid having to detect lib/cli usage

blocked on mui/mui-x#16112

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Jan 8, 2025
@mui-bot
Copy link

mui-bot commented Jan 8, 2025

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 4b3f0a4

@Janpot Janpot changed the title [code-infra] Avoid mixing cjs and esm for docs config [code-infra] Remove commonjs imports in docs Jan 8, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 13, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 20, 2025
@Janpot Janpot marked this pull request as ready for review January 20, 2025 16:08
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 21, 2025
"./loader": "./loader.mjs",
"./prism": {
"types": "./prism.d.mts",
"require": "./prism.cjs",
Copy link
Member

Choose a reason for hiding this comment

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

Is their a reason to continue exporting a cjs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, regression tests at the moment still select the cjs variant. I'm moving them to vite in follow-up.

test: /\.(js|ts|tsx)$/,
// prism.js blocks @mui/internal-markdown/prism from being interpreted as ESM in this build.
exclude: /node_modules|prism\.js/,
test: /\.(js|mjs|ts|tsx)$/,
Copy link
Member

Choose a reason for hiding this comment

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

If I understand corectly, this is to be able to run test on markdown new files.

I've seen similar update on the mui-x PR (mui/mui-x#16112) but no .mjs files added. Is their a specific reason?

Copy link
Member Author

@Janpot Janpot Jan 21, 2025

Choose a reason for hiding this comment

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

If I understand corectly, this is to be able to run test on markdown new files.

It's to include the mjs files of @mui/internal-markdown in the compilation.

...Is their a specific reason?

Yes, I added it there as well, only in X, this rule is configured in webpackBaseConfig for some reason. After I move the tests to vite, I think only eslint will still be using this config for its aliases, so we should be able to remove the rule altogether.

test/regressions/webpack.config.js Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 21, 2025
@Janpot Janpot merged commit a56abff into mui:master Jan 21, 2025
22 checks passed
@Janpot Janpot deleted the remove-cjs branch January 22, 2025 14:38
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