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

Move conditional useInsertionEffect declarations into separate package #2867

Merged
merged 9 commits into from Aug 30, 2022

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Aug 26, 2022

Why?

Sometimes people bundle Emotion in their libraries~ (for whatever reasons, one such example would be @storybook/theming) and while doing so their bundlers/minifiers replace our carefully crafted React['useInsertion' + 'Effect'] with just React.useInsertionEffect. That, in turn, can create problems when consuming such bundles with webpack and some other tools (it's also the reason why we are using this weird pattern in the first place). Most notably, webpack errors with:

require function is used in a way in which dependencies cannot be statically extracted

It's hard to control bundlers/minifiers to leave this pattern alone, so I think that what we can offer to people with such setups is this package that they will be able to externalize in their setups. The main reason for bundling Emotion etc is to isolate its context (create its copy) to avoid conflicts with 3rd party code and stuff like that - and it's not to actually make a copy of everything.

fixes #2738

@changeset-bot
Copy link

changeset-bot bot commented Aug 26, 2022

🦋 Changeset detected

Latest commit: 273192d

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

This PR includes changesets to release 3 packages
Name Type
@emotion/react Patch
@emotion/styled Patch
@emotion/use-insertion-effect-with-fallbacks 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

Comment on lines +11 to +13
export const useInsertionEffectAlwaysWithSyncFallback = !isBrowser
? syncFallback
: useInsertionEffect || syncFallback
Copy link
Member Author

Choose a reason for hiding this comment

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

Please be extra careful when reviewing this as the shape of this has slightly changed when I moved this. I think that the current code is functionally equivalent to the old version though.

@@ -0,0 +1,38 @@
{
"name": "@emotion/use-insertion-effect-with-fallbacks",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be *-with-fallback or *-with-fallbacks? 😅 #realproblems

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 27, 2022

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 273192d:

Sandbox Source
Emotion Configuration

@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #2867 (273192d) into main (28522cd) will increase coverage by 0.05%.
The diff coverage is 92.85%.

Impacted Files Coverage Δ
...s/use-insertion-effect-with-fallbacks/src/index.js 88.88% <88.88%> (ø)
packages/react/src/class-names.js 100.00% <100.00%> (ø)
packages/react/src/emotion-element.js 100.00% <100.00%> (ø)
packages/react/src/global.js 100.00% <100.00%> (+1.75%) ⬆️
packages/styled/src/base.js 100.00% <100.00%> (ø)

@gyhaLabs
Copy link

I have this very similar issue in our project. We are using a common UI library (of our own) - which uses material UI with @emotion/react - which may be a very common combo. (See the installation doc for the material UI: https://mui.com/material-ui/getting-started/installation/).

When I am including this common/shared UI package in the main project, it won't do the build/deployment and the GitLab pipline fails with the error below.

image

This is a valid use-case, right? It is a similar issue with the above mentioned, right @Andarist?

Thanks.

@Andarist
Copy link
Member Author

@gyhaLabs I think this is very much related. My strong suspicion is that you are bundling Emotion (effectively creating its copy) and that, together with your UI library, is later consumed together by your consumers.

@@ -0,0 +1,44 @@
{
"name": "@emotion/use-insertion-effect-with-fallbacks",
"version": "1.0.0",
Copy link
Member

@emmatown emmatown Aug 29, 2022

Choose a reason for hiding this comment

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

Suggested change
"version": "1.0.0",
"version": "0.0.0",

+a changeset

@Andarist Andarist merged commit 89b6dbb into main Aug 30, 2022
@Andarist Andarist deleted the use-insertion-effect-with-fallbacks branch August 30, 2022 08:41
@github-actions github-actions bot mentioned this pull request Aug 30, 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.

Issues in React 17 with terser and minifiers (rewrite property access using dot notation) (useInsertionEffect)
3 participants