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

Drop compat plugin #3294

Open
wants to merge 53 commits into
base: next
Choose a base branch
from
Open

Drop compat plugin #3294

wants to merge 53 commits into from

Conversation

Andarist
Copy link
Member

closes #2836

@Andarist Andarist requested a review from emmatown December 14, 2024 17:49
Copy link

changeset-bot bot commented Dec 14, 2024

🦋 Changeset detected

Latest commit: b98f72b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@emotion/styled Major
@emotion/cache Major
@emotion/react Major
@emotion/css Major
@emotion/primitives-core Major
@emotion/server Major
@emotion/native Major
@emotion/primitives Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codesandbox-ci bot commented Dec 14, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

While I agree that we should do this eventually, I don't think we should merge this now and if we want to ship this, we should at minimum:

  • Add warnings in Emotion 11 when the compat plugin changes stuff that explains why it's changing (to align closer with standard css nesting1) and how to fix it (probably write &:pseudo unless you actually meant the standard meaning in which case write *:pseudo to silence this warning)
  • Change our types from suggesting pseudo selectors in object types to either not suggesting them at all or suggesting &:pseudo since we're effectively suggesting people write the thing they almost certainly do not intend

And tbh even if we do those things, I think it might be good to keep the compat plugin doing the "wrong" thing and warning for the duration of v12 to ease migration for longe.r (though I don't feel as strongly about this)

imo, fixing this without doing those things would cause more pain that it would solve since people can deal with #2836 by just writing *:where(...) instead of :where(...) 2 and that's easy to deal with since it will be broken at the time someone is writing code so authors will notice it and work around it but "properly" fixing this means breaking code that people have already written and not in a simple kind of break where it's just "there's a bunch of errors, fix all the errors and you're good" but "there are no warnings, only some of it is broken in subtle ways, you can probably find and replace your way to get most of it done but you might not have actually found all the cases".

Also, I think "we do nothing about this for v12" is also explicitly a completely fine answer if that's the reality time-wise and is preferable to shipping this with a bad migration path.

Footnotes

  1. Though it still isn't actually the same as standard CSS nesting since that would require using :is to have the exact same specificity stuff but we wouldn't want to do that because browser support. We of course don't need to mention this in the warning, just wanted to note it.

  2. Unless I'm missing something though no one has suggested that from https://github.com/emotion-js/emotion/pull/3210#issuecomment-2237858626 so far

Base automatically changed from use-react19 to next December 15, 2024 11:01
@emmatown emmatown mentioned this pull request Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants