-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
🦋 Changeset detectedLatest commit: 273192d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
export const useInsertionEffectAlwaysWithSyncFallback = !isBrowser | ||
? syncFallback | ||
: useInsertionEffect || syncFallback |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
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:
|
Codecov Report
|
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. This is a valid use-case, right? It is a similar issue with the above mentioned, right @Andarist? Thanks. |
@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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"version": "1.0.0", | |
"version": "0.0.0", |
+a changeset
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 craftedReact['useInsertion' + 'Effect']
with justReact.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: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