-
-
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
Remove peer dependency on @babel/core
from most packages
#2985
Remove peer dependency on @babel/core
from most packages
#2985
Conversation
🦋 Changeset detectedLatest commit: 42451cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
28d6184
to
24a5e99
Compare
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 42451cd:
|
Codecov Report
|
@@ -18,7 +18,6 @@ | |||
], | |||
"dependencies": { | |||
"@babel/helper-module-imports": "^7.16.7", |
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.
this depends on @babel/types
but I guess that it should be mostly OK since those are just factories for simple objects and they shouldn't rely on identity of any class or anything like that
"peerDependencies": { | ||
"@babel/core": "^7.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.
This feels off but I guess that it's not a big deal if we omit it from here - we mostly use Babel through what we are given by the caller anyway.
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.
i'm also still curious though - is this removal technically needed to fix the mentioned issues? Isn't inling@babel/plugin-syntax-jsx
already sufficient?
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.
This comment indirectly answers this question. This is somewhat needed today because we have a direct dependency on @emotion/babel-plugin
in some packages.
What do you think about moving macros completely out of the main packages in Emotion 12 (not that I currently plan to release a new version)? |
Maybe? Could just make the babel plugin an optional peer dependency though? (Note currently @babel/core isn't an optional peer dependency regardless of those peerDependenciesMeta since it's a non-optional peerDependency of @babel/plugin-syntax-jsx) |
Hm, isn't this a problem with the package manager? If we optionally depend in A on B that depends non-optionally on C that shouldn't transitively make C a required dependency of A 🤔 |
We non-optionally depend on B (@babel/plugin-syntax-jsx) in this case |
I don't get it... |
|
Ah, I see... I was focusing on the |
4f84220
to
42451cd
Compare
Amazing this was fixed!! This will get rid of our last |
Closes #2921
Closes #2660
So the only reason there is a peer dependency on
@babel/core
is because of@babel/plugin-syntax-jsx
. I don't really think doing we even need to make@emotion/babel-plugin
inherit@babel/plugin-syntax-jsx
tbh since the only case it would matter is if people were running@emotion/babel-plugin
without a JSX transform but just to not change the behaviour, I've inlined@babel/plugin-syntax-jsx
. I think the cost of inlining it is more than worth it for getting rid of a peer dependency warning for most users.