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
Let path.remove()
remove IfStatement.alternate
#14833
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52717/ |
Tip: Currently our active branch is |
Can you rebase on the |
Special-casing "consequent" makes sense, but there is no need to deviate from the normal `remove()` behavior for "alternate", which is permitted to be null.
Ah, I didn't realize GitHub didn't do that automatically. Updated! |
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! Can you add a new test case to packages/babel-traverse/test/removal.js
?
One test verifies the behavior fixed in babel#3583 (removing consequent does not make it null) and the other babel#14833 (removing alternate *does* make it null).
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!
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
path.remove()
remove IfStatement.alternate
Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com> Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
While writing a transformation plugin, I would expect calling
remove()
on anelse
clause to remove theelse
, not replace it with an empty block. Special-casing "consequent" makes sense, because it is not permitted to be null, but there is no need to deviate from the normalremove()
behavior for "alternate".Let me know if tests are needed for this since it's a small change.