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

Always print parentheses around arrow function parameters #13202

Closed
wants to merge 3 commits into from

Conversation

Zalathar
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #13198
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Existing tests updated
Documentation PR Link
Any Dependency Changes? No
License MIT

Some arrow functions can safely be printed without parentheses around a single identifier parameter.

However, there are many factors that can make this unsafe (e.g. type annotations, comments), and several generator features that can silently introduce edge cases (retainLines, auxiliaryCommentBefore/auxiliaryCommentAfter). These make it hard to be confident that printing the bare identifier really is safe.

This PR therefore makes the generator always print the parentheses, even when they aren't strictly necessary, greatly simplifying the implementation.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 25, 2021

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 25, 2021

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 ddb4f9e:

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

@Zalathar
Copy link
Contributor Author

I have an alternate version of this change that tries to retain no-parens in some cases, and instead just tries to make the logic simpler and more robust.

But I like the idea of just always spending two characters to guarantee correct output.

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.

Looking at the output, maybe we could still keep the single identifier param, no async, no comments, no types case (which is one of the most common ones) parens-less, or at least do it when the compact: true option is set.

It should still be safe, since it doesn't need to check loc and accidental line breaks don't cause problems.

(x): %checks => x !== null;
Copy link
Member

Choose a reason for hiding this comment

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

Oh this was a bug

@Zalathar
Copy link
Contributor Author

OK, I'll polish up the version that tries to keep the no-parens in simple cases, and see what that looks like.

@Zalathar
Copy link
Contributor Author

I opened a separate PR for the alternate version at #13204, to avoid having to force-push back and forth between the two versions.

@existentialism existentialism added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Apr 26, 2021
@Zalathar
Copy link
Contributor Author

Closed in favour of #13204.

@Zalathar Zalathar closed this Apr 26, 2021
@Zalathar Zalathar deleted the arrow-function-param-parens branch April 27, 2021 09:08
@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 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 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 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.

[Bug]: Edge-case exception while printing single-parameter arrow function
4 participants