useInsertionEffect when it's available
#2600
Conversation
…ersion, also run test suite with React 18
…n `@emotion/react` and `@emotion/styled``
… used with RTL cause it's using act
🦋 Changeset detectedLatest commit: 7be5a95 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
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. Latest deployment of this branch, based on commit 7be5a95:
|
Codecov Report
|
| @@ -91,52 +91,6 @@ exports[`hydration only inserts rules that are not in the critical css 3`] = ` | |||
| box-shadow: -15px -15px 0 0 aqua,-30px -30px 0 0 cornflowerblue; | |||
| } | |||
|
|
|||
| @font-face { | |||
There was a problem hiding this comment.
those were duplicated in the snapshot - I'm not entirely sure why, but the test setup here (and in other 3 tests that very similar) was a little bit convoluted, with using JSDOM manually etc. I've migrated those tests to disableBrowserEnvTemporarily strategy and this has "fixed" the problem accidentally.
| @@ -157,9 +111,7 @@ exports[`renderStylesToString renders large recursive component 1`] = ` | |||
| > | |||
| .some-key-127stik{color:hotpink;} | |||
| </style> | |||
| <div class="some-key-127stik" | |||
| data-reactroot | |||
There was a problem hiding this comment.
there is a comment somewhere about this - React 18 is not using this attribute any longer (I'm still confirming this with React 18 Working Group), so I'm normalizing this to avoid snapshot mismatched between versions
| @@ -49,7 +54,32 @@ export const createEmotionProps = (type: React.ElementType, props: Object) => { | |||
| return newProps | |||
| } | |||
|
|
|||
| const Noop = () => null | |||
| const Insertion = ({ cache, serialized, isStringTag }) => { | |||
There was a problem hiding this comment.
the exact same thing got introduced in @emotion/styled, the code was already somewhat duplicated in both packages anyway, and sharing it now is not super easy cause of the peer deps constraints etc
| const Insertion = ({ cache, serialized, isStringTag }) => { | ||
| registerStyles(cache, serialized, isStringTag) | ||
|
|
||
| const rules = useInsertionEffectMaybe(() => |
There was a problem hiding this comment.
One thing to consider - it would be cool to flatten the React tree cause this Insertion component is not needed. Or rather, it is somewhat needed:
- it creates a "slot" so we avoid SSR mismatches when
useIdis used. But that is only a requirement when relying on 0-config and I think that a lot of people are not actually using this feature. With most SSR-oriented APIs we don't have to renderstyleelements "inline" and in those setups there is no need for this extra "slot" - it preserves injection order. We know that injection order is not something that people should rely on but we'd like to make it as easy as possible for them to migrate
It seems that we can't easily work around the second "requirement' unless we provide a way for people to configure injectWhenever. I think the plan is to just make this a default in the next major version though - so the question is, should we allow people to flatten their React trees with a flag or something?
There was a problem hiding this comment.
I think for now, let's leave it out.
No description provided.