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

[next] Wrap all generated CSS with a cascade layer (WIP) #226

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Sep 10, 2024

One way to fix #199

Notes:

  • Does this have unwanted side-effects?
  • TODO: sourcemaps. Can we get away with them shifting down 1 line for the sake of a quick fix? => let's run with it for now.
  • Does this belong in wyw-in-js?

Closes mui/material-ui#43705

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Sep 10, 2024
@Janpot Janpot changed the title [next] Wrap all CSS with a cascade layer (WIP) [next] Wrap all generated CSS with a cascade layer (WIP) Sep 10, 2024
@DiegoAndai
Copy link
Member

Does this have unwanted side-effects?

We have visual regression tests running with Pigment on the Material UI repo, we can create a draft PR pointing to these builds to check if anything unexpected is happening there. It probably won't cover all edge cases but it would be helpful.

Can we get away with them shifting down 1 line for the sake of a quick fix?

Why do you consider this to be a quick fix?

If it is a quick fix, can users implement it on their side? We could propose it as a workaround until we agree on a definitive solution.

Does this belong in wyw-in-js?

If it is, let's try to fix it on our side anyway. We can open an issue on their repo and refactor it later, but let's not block a fix on our side.

@Janpot
Copy link
Member Author

Janpot commented Sep 11, 2024

We have visual regression tests running with Pigment on the Material UI repo, we can create a draft PR pointing to these builds to check if anything unexpected is happening there. It probably won't cover all edge cases but it would be helpful.

Would this cover it? Seems to pass.

Why do you consider this to be a quick fix?

Because it's just rewriting the css without considering the source maps. This means that the links in devtools will be off by one line. It may be quite a bit more involved to write this in a way that sourcemaps are transformed as well. It all comes down to how much effort we want to pour down into fixing this. For now it's probably good enough as a fix.

If it is a quick fix, can users implement it on their side?

I don't think so, this is just a bug on our side.

@brijeshb42
Copy link
Contributor

I am not sure how fool proof the fix is and if wouldn't cascade into some other bug.
Let me check this change with some calls to globalCss api.

@Janpot
Copy link
Member Author

Janpot commented Sep 11, 2024

What do you mean with "globalCss api"?

I've tested by adding a ./global.css overriding some properties on Mui classes. That still works since unlayered styles are more specific than layered styles.

If this is not the fix, we're going to have to brainstorm 😄. Next.js recommends cascade layers as a way to get around this issue.

@@ -284,6 +283,19 @@ export const plugin = createUnplugin<PigmentOptions, true>((options) => {
}

let { cssText } = result;

const slug = slugify(cssText);
Copy link
Contributor

@brijeshb42 brijeshb42 Sep 11, 2024

Choose a reason for hiding this comment

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

Can you move the whole thing at the end of the if (isNext) check at line https://github.com/mui/pigment-css/blob/master/packages/pigment-css-unplugin/src/index.ts#L301

since we mainly want to do this for next.js and not webpack (unless someone reports it for webpack as well).

@brijeshb42
Copy link
Contributor

brijeshb42 commented Sep 11, 2024

Does this have unwanted side-effects?

We'll have to see if someone reports any new issues. It fixes the current bug.

TODO: sourcemaps. Can we get away with them shifting down 1 line for the sake of a quick fix? => let's run with it for now.

Sourcemaps are working without any issues since the linkage is through the generated class names which we are not touching.

Does this belong in wyw-in-js?

Not really since wyw does not have any nextjs specific solution. If it had @wyw/nextjs, then yes.

Copy link
Contributor

@brijeshb42 brijeshb42 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@Janpot Janpot merged commit 2a45a3a into mui:master Sep 11, 2024
12 checks passed
@Janpot Janpot deleted the fix-next-layout-order branch September 11, 2024 14:45
This was referenced Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component styles are overridden when rendered inside Next.js layout
3 participants