-
Notifications
You must be signed in to change notification settings - Fork 261
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
fix: make styled-jsx/babel plugin respect the source type #684
Conversation
[t.importDefaultSpecifier(t.identifier(STYLE_COMPONENT))], | ||
t.stringLiteral(styleModule) | ||
: 'styled-jsx/style', | ||
{ nameHint: STYLE_COMPONENT} |
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 not 100% sure what will happen if there is an existing variable in scope that has the same name as STYLE_COMPONENT
(_JSXStyle
), but I think the addDefault
helper will modify the nameHint
to avoid a collision. Ideally, no nameHint
would be used and the automatic name of the injected default import would be used throughout the code instead of a hardcoded constant. This way a user could have a _JSXStyle
variable name in the same scope, no problem.
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.
All code branches that call this already check if STYLE_COMPONENT
is in scope, so this should "just work".
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.
There is a remote chance that _JSXStyle
is in the source code but it is not styled-jsx's i.e. the user defined a binding with that name which will conflict with STYLE_COMPONENT
.
If you want to fix this, you should probably generate a unique identifier from STYLE_COMPONENT
, put it in state and pass it to helpers if necessary but I probably I wouldn't bother in this PR – it's been like that for 5-ish years
Happy to land this if you update it! |
I edited the PR title to conform with the failing checks, but the
Where you just referring to the PR title? This PR is good-to-go. |
I swore I saw a merge conflict, but I guess not. |
🎉 This PR is included in version 3.3.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #680 .
@babel/helper-module-imports
should be used when injecting imports, as it will use eitherimport
orrequire
syntax depending if the BabelsourceType
for the file ismodule
(ESM) orscript
(CJS).