Skip to content
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 printing of single-param async arrow function with comments #13136

Merged

Conversation

nwalters512
Copy link
Contributor

@nwalters512 nwalters512 commented Apr 11, 2021

Q                       A
Fixed Issues? Fixes #12500
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Any Dependency Changes? No
License MIT

This PR ensures that the parameter of a single-parameter async arrow function is printed inside parentheses if the parameter has leading or trailing comments. Without the parentheses, the generated code is not interpreted correctly and results in a syntax error. See #12500 for the original report of this problem.

It's not clear to me why so much care went into ensuring that single-argument functions don't include unnecessary parentheses - this code could be simplified and made much more robust if we were willing to unconditionally add parentheses around all arrow function parameters. I'd be happy to make that change, but I figured I'd check in with the maintainers first in case there's a good reason that parentheses are omitted if possible.

I also added support for an OVERWRITE environment variable for the babel-generator tests - the instructions in #12500 (comment) said that this should work, but support was missing from this particular test suite. Happy to pull this into a separate PR if requested.

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45138/

@codesandbox-ci
Copy link

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 075bff6:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo added pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 12, 2021
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Moving the OVERWRITE thing in a separate PR allows us listing it in the changelog as an Internal change, but imo it's ok to keep it here and not list it.

@nicolo-ribaudo
Copy link
Member

It's not clear to me why so much care went into ensuring that single-argument functions don't include unnecessary parentheses - this code could be simplified and made much more robust if we were willing to unconditionally add parentheses around all arrow function parameters. I'd be happy to make that change, but I figured I'd check in with the maintainers first in case there's a good reason that parentheses are omitted if possible.

If we find another parens-related bug we can decide to stop using this logic and always printing them. That code has been like that for so long that I have no idea why we initially didn't always print the parens 🤷

@JLHwung JLHwung merged commit 30f93b3 into babel:main Apr 12, 2021
@nwalters512 nwalters512 deleted the fix/async-arrow-function-with-comments branch April 12, 2021 14:38
@hzoo
Copy link
Member

hzoo commented Apr 12, 2021

yeah agreed with @nicolo-ribaudo, simplifying it sounds good to me.

I think the reason why we probably did it is if you wanted to save bytes (though it would be covered by the minifier). We also removed a lot of other options from generator (quotes/tabs, etc) since it's not the focus and can replace it with recast/prettier for codemods instead.

@nwalters512
Copy link
Contributor Author

@hzoo would you rather wait til another bug is found like @nicolo-ribaudo suggested? Otherwise, I can make that change now while the context is still fresh in my head.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unary async arrow misprinted as concise async arrow when the binding identifier has leading comments
5 participants