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

fix: styled-engine new internals #220

Merged
merged 16 commits into from
Sep 3, 2024
Merged

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Aug 30, 2024

Update to match the changes in mui/material-ui#43412

@romgrk
Copy link
Contributor Author

romgrk commented Aug 30, 2024

Duplicate of #219

@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label Aug 31, 2024
@@ -8,3 +8,4 @@ export { default as createUseThemeProps } from './createUseThemeProps';
export { default as internal_createExtendSxProp } from './createExtendSxProp';
export { default as useTheme } from './useTheme';
export { default as globalCss } from './globalCss';
export { default as internal_styledEngineMockup } from './styledEngineMockup';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be part of the public api (base module export) but can come from a path like @pigment-css/react/internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the convention for internal_createExtendSxProp right above. I'll move it to a new internal folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add this path to the exports in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried it but it wasn't enough, not sure how the different (tsconfig|tsup).* files interact, any advice to setup the build properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I think I got it.

@@ -118,13 +120,7 @@ const pigmentOptions = {
...context,
require: (id) => {
if (id === '@mui/styled-engine' || id === '@mui/styled-engine-sc') {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed here. I'll clean this up separately since it's not a package.

Copy link
Contributor

Choose a reason for hiding this comment

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

One last change needed is an entry it .gitignore and include internal in package.json's files key.

@romgrk
Copy link
Contributor Author

romgrk commented Sep 2, 2024

Should be good now.

@romgrk
Copy link
Contributor Author

romgrk commented Sep 3, 2024

@brijeshb42 Can the netlify CI be ignored? And if so, could you merge this PR? I don't have write access to this repo.

I'm also not sure how pigment-css releases work, when will this be released? If it's released by next week then I'll look into finalizing the PR on the core.

@brijeshb42
Copy link
Contributor

brijeshb42 commented Sep 3, 2024

I'll release it this week itself. Maybe by tomorrow.

@brijeshb42 brijeshb42 merged commit 4703ccc into mui:master Sep 3, 2024
11 of 12 checks passed
@romgrk
Copy link
Contributor Author

romgrk commented Sep 3, 2024

Ah I think I need this to be published to merge the core one, or the tests won't pass. Much appreciated if you can release it tomorrow.

@romgrk romgrk deleted the fix-mui-internals branch September 3, 2024 13:42
@brijeshb42
Copy link
Contributor

@romgrk Can you check your changes in core with this CI package to make sure the changes work before I actually publish ?

@romgrk
Copy link
Contributor Author

romgrk commented Sep 3, 2024

How would I go about setting that up? I see the http CSB package but iiuc that only works on CSB.

I've checked by linking my local pigment repo to my local material repo. With the old branch if fails due to the internal API mismatch, and with the new branch if fails due to unrelated errors, so I'm assuming it's picking up the right thing.

@brijeshb42
Copy link
Contributor

You can use the csb urls in the top level package.json's resolutions key and then run the build.
Make sure to add all the 4 packages in the key.
If youve already tested then 👍

@brijeshb42
Copy link
Contributor

Published v0.0.22. Please check your changes with this version.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants