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] Fix and update bundling fixtures #43709

Merged
merged 56 commits into from
Sep 30, 2024
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Sep 11, 2024

Bundling tests were broken since we moved to pnpm. Bundling tests will be instrumental in verifying new package layouts. That PR should fix the currently failing node ESM test.

Initially I tried a new setup based on workspace:* dependencies and rely on pnpm linking to build against build output. Looks like this interferes too much with the Next.js build process at the moment.

For now I settled on packing the MUI packages with npm pack (to remove the workspace:* links) and installing them with pnpm overrides.

I also added a commonjs bundling test. It's quick and dirty converting the ESM fixture imports to require calls.

Updated the README with new instructions

All tests are passing now (except the ESM test, as expected):

Screenshot 2024-09-25 at 18 01 26

Solved a few problems with our playwright installation along the way

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

mui-bot commented Sep 11, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 5c11a76

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 17, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 2024
@Janpot Janpot requested a review from a team September 25, 2024 16:42
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 27, 2024
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.

Good job updating these. 👍
LGTM. 👌

}

async function run({ packages, outDir, concurrency }: RunOptions) {
const allWorkspaces: WorkspaceDefinition[] = await $`pnpm m ls --depth -1 --json`.then((result) =>
Copy link
Member

Choose a reason for hiding this comment

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

What does pnpm m do?

Copy link
Member Author

@Janpot Janpot Sep 30, 2024

Choose a reason for hiding this comment

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

🤔 From pnpm m --help

Concurrently performs some actions in all subdirectories with a package.json (excluding node_modules). A pnpm-workspace.yaml file may be used to control what directories are searched for packages.

Looks like it's the same as -r, will update

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find it in their docs, didn't think of running --help.
Anyway, I'd recommend using longhand switches and parameters in our scripts so they are more self-explanatory. Shorthands are good when interacting with the CLI manually to save a few keystrokes.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 30, 2024
@Janpot Janpot merged commit 2ea7cf6 into mui:master Sep 30, 2024
19 checks passed
@Janpot Janpot deleted the bundling-fixtures branch October 1, 2024 09:23
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.

4 participants