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 arrow transformation when arguments
is defined as variable
#12344
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45949/ |
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 2cbb3ec:
|
arguments
is defined as var
f38831d
to
c353aa4
Compare
The CI failure is a bug in a dependency. |
So do we have to wait for the patch release of the dep? |
Not to land this PR, no. |
5330ae1
to
6e456db
Compare
The tests are passing locally 😕 |
Since the failures seems realted to this PR, try rebasing on |
6e456db
to
98e7154
Compare
...ges/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/arguments/output.js
Outdated
Show resolved
Hide resolved
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.
Hey I'm sorry that we forgot about this PR for four months 😅
I left some comments, but if you aren't interested anymore since it's been so long I can make the necessary changes.
...ges/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/arguments/output.js
Outdated
Show resolved
Hide resolved
I will update the PR 👍 |
98e7154
to
c4b1d30
Compare
@nicolo-ribaudo I have updated the PR, please take a look. |
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 realized I had this pending comment since your last commit, and I forgot to send the review 😅
...ages/babel-plugin-transform-arrow-functions/test/fixtures/arrow-functions/arguments/input.js
Outdated
Show resolved
Hide resolved
packages/babel-core/test/fixtures/transformation/misc/regression-2364/output.js
Outdated
Show resolved
Hide resolved
16e82c2
to
2cbb3ec
Compare
if (thisEnvFn.scope.path.isProgram()) { | ||
return t.conditionalExpression( | ||
t.binaryExpression( | ||
"===", | ||
t.unaryExpression("typeof", args()), | ||
t.stringLiteral("undefined"), | ||
), | ||
thisEnvFn.scope.buildUndefinedNode(), | ||
args(), | ||
); |
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 added this code to avoid the error described in #7673. It still isn't 100% correct since sometimes it doesn't throw the error if arguments
is undeclared, but at least it doesn't throw for valid code.
arguments
is defined as vararguments
is defined as variable
Thanks for the update 👍 |
…el#12344) * fix: arrow-fn transformation when 'arguments' is defined as var * fix: tests * refactor: code * Review by @nicolo-ribaudo Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Fixes #11385