-
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 dynamic interpolations #486 #536
Conversation
Can you rebase with master? |
not being added in time for other transformers to edit them
1ac902f
to
396e2a0
Compare
It should be rebased to master now |
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.
thanks for looking into this! I will need to run a few tests to see if such a change would slow down builds
src/babel.js
Outdated
JSXOpeningElement(path, state) { | ||
const el = path.node | ||
const { name } = el.name || {} | ||
let state = {} |
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 is not "cool" we should call traverse
with the state
and opts
we get from the plugin https://github.com/babel/babel/blob/aaec2cd51dd24a1ecf52b82e0bbad0a5ccd60b6f/packages/babel-traverse/src/index.js#L14-L20
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.
Yeah that was a bit hacky sorry, it's updated to pass the state the correct way now
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.
No worries and no need to be sorry! Thank you
test/index.js
Outdated
const { code } = await transform( | ||
'./fixtures/dynamic-this-value-in-arrow.js', | ||
{ | ||
presets: ['@babel/preset-env'] |
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.
maybe install @babel/plugin-transform-arrow-functions
and run it like this:
const { code } = await transform(
'./fixtures/dynamic-this-value-in-arrow.js',
{
plugins: [
'@babel/plugin-transform-arrow-functions',
plugin,
]
}
)
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.
That works, I updated it to just use the @babel/plugin-transform-arrow-functions
plugin instead of the @babel/preset-env
preset
Probably this is not a big deal after all. I ran zeit.co and in dev it is built in 87ms. @timneutkens FYI we are switching to visiting the |
transform-arrow-functions in dynamic `this` test
thanks again @ijjk I'll cut a release tomorrow morning (EU time) |
Ok sounds good. Thanks for getting this merged! |
After digging into this issue it looks like a change from
babel-plugin-transform-es2015-arrow-functions
to@babel/plugin-transform-arrow-functions
in babel 7's@babel/preset-env
broke the dynamic interpolation. If you had thespec
option set totrue
on the@babel/plugin-transform-arrow-functions
you wouldn't have encountered this error, but since the default isfalse
most people using babel 7 and dynamic interpolation in this way would encounter an error.After talking in Babel's slack it seems one of the ways to fix this is to make sure to traverse the JSX elements before
@babel/plugin-transform-arrow-functions
has a chance to transform preventingthis
from being changed before theclassName
attribute is added to the elements.This is based off my
upgrade-babel
branch so that should probably be finished first. I wasn't sure if you wanted this fix on a separate pull request or not so if you want me to just push it on the other branch and merge it on that pull request let me know. Fixes #486