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

useInsertionEffect when it's available #2600

Merged
merged 13 commits into from Feb 19, 2022
Merged

useInsertionEffect when it's available #2600

merged 13 commits into from Feb 19, 2022

Conversation

Andarist
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Dec 20, 2021

🦋 Changeset detected

Latest commit: 7be5a95

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

This PR includes changesets to release 4 packages
Name Type
@emotion/react Minor
@emotion/styled Minor
@emotion/utils Minor
@emotion/jest Minor

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 20, 2021

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:

Sandbox Source
Emotion Configuration

@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #2600 (f820725) into main (242f7d8) will decrease coverage by 0.01%.
The diff coverage is 96.70%.

❗ Current head f820725 differs from pull request most recent head 7be5a95. Consider uploading reports for the commit 7be5a95 to get more accurate results

Impacted Files Coverage Δ
packages/react/src/global.js 98.24% <80.00%> (-1.76%) ⬇️
packages/react/src/useInsertionEffectMaybe.js 88.88% <88.88%> (ø)
packages/styled/src/useInsertionEffectMaybe.js 88.88% <88.88%> (ø)
packages/jest/src/utils.js 96.53% <100.00%> (+0.06%) ⬆️
packages/react/src/class-names.js 100.00% <100.00%> (ø)
packages/react/src/emotion-element.js 100.00% <100.00%> (ø)
packages/server/test/util.js 98.18% <100.00%> (+1.57%) ⬆️
packages/styled/src/base.js 100.00% <100.00%> (ø)
packages/utils/src/index.js 100.00% <100.00%> (ø)
scripts/test-utils/src/index.js 93.33% <100.00%> (+6.66%) ⬆️

@@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Choose a reason for hiding this comment

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

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 }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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(() =>
Copy link
Member Author

Choose a reason for hiding this comment

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

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 useId is 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 render style elements "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?

Copy link
Member

Choose a reason for hiding this comment

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

I think for now, let's leave it out.

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.

Looks good to me, just needs changesets

@Andarist Andarist merged commit 2f27156 into main Feb 19, 2022
@Andarist Andarist deleted the use-insertion-effect branch February 19, 2022 10:05
@github-actions github-actions bot mentioned this pull request Feb 19, 2022
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.

None yet

2 participants