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.

Sign up for GitHub

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

Jump to bottom

[system][zero] Move zero runtime related packages #40426

Merged
merged 11 commits into from
Jan 8, 2024
Merged

[system][zero] Move zero runtime related packages #40426

merged 11 commits into from
Jan 8, 2024

Conversation

brijeshb42
Copy link
Contributor

from the separate repo

@brijeshb42 brijeshb42 added the package: system Specific to @mui/system label Jan 4, 2024
@mui-bot
Copy link

mui-bot commented Jan 4, 2024 •
edited

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against fc7fc61

@zannager zannager requested a review from Janpot January 4, 2024 11:03
@Janpot
Copy link
Member

Janpot commented Jan 4, 2024

@brijeshb42 I don't have the bandwidth to go through all the code. I'll try to go through it diagonally. I assume the business logic has been reviewed on its original repo? Did you have to change much compared to your original setup? Anything you had to hack around or noticed any rough edges?

@Janpot Janpot requested a review from mnajdova January 4, 2024 11:24
@brijeshb42
Copy link
Contributor Author

@Janpot Yes. Main logic has already been reviewed. The change specifically for the zero runtime packages is that it still uses the same build step (as in the separate repo) which uses tsup and does not use standalone build folder with package.json.
I am also investigating why this is failing.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 4, 2024
@brijeshb42 brijeshb42 force-pushed the zero-pnpm branch 2 times, most recently from 3307c23 to e5ee3bb Compare January 5, 2024 05:34
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 5, 2024
@brijeshb42 brijeshb42 force-pushed the zero-pnpm branch 2 times, most recently from cf6f251 to 7fb6c54 Compare January 5, 2024 07:40
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 5, 2024
@Janpot
Copy link
Member

Janpot commented Jan 5, 2024

The change specifically for the zero runtime packages is that it still uses the same build step (as in the separate repo) which uses tsup

Will we keep it that way or migrate?

@brijeshb42
Copy link
Contributor Author

The plan is to keep it that way in the short term to focus on the actual development and migrate away in the long term.

@mnajdova mnajdova changed the title [system][zero]: Move zero runtime related packages [system][zero] Move zero runtime related packages Jan 5, 2024
brijeshb42
brijeshb42 commented Jan 5, 2024
View reviewed changes
.circleci/config.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 7, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2024
@michaldudak michaldudak mentioned this pull request Jan 8, 2024
Merged
@brijeshb42
Copy link
Contributor Author

@mnajdova This PR is now ready for review after all the build related problems are solved.

@Janpot Janpot mentioned this pull request Jan 8, 2024
Open
mnajdova
mnajdova reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

From the initial testing, everything seems to work 🎉 I left two thins to be checked before merging.

After the merge, we need to cleanup the example app and provide structure for how we are going to validate that the migrated components work.

.eslintignore Outdated Show resolved Hide resolved
packages/mui-material/package.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2024
@brijeshb42 brijeshb42 enabled auto-merge (squash) January 8, 2024 14:17
@brijeshb42 brijeshb42 merged commit dcc8c8f into mui:master Jan 8, 2024
19 checks passed
@brijeshb42 brijeshb42 deleted the zero-pnpm branch January 8, 2024 14:52
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
@brijeshb42 @mnajdova
[system][zero] Move zero runtime related packages (mui#40426)
74d3a83
@oliviertassinari oliviertassinari added the package: pigment-css Specific to @pigment-css/* label Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak left review comments

@mnajdova mnajdova mnajdova approved these changes

@Janpot Janpot Awaiting requested review from Janpot

Assignees
No one assigned
Labels
package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brijeshb42 @mui-bot @Janpot @mnajdova @michaldudak @oliviertassinari