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 comments before => #15160

Merged
merged 2 commits into from Nov 8, 2022
Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Nov 8, 2022

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

This fix conflicts with the fix proposed in #15159, however I believe that it solves the problem in a better way: we must keep the correct _noLineTerminator to make sure to disallow any multiline comment right before =>.

All these cases with multi-line comments before => cannot happen just by parsing+printing the source code, but can happen if plugins modify the AST. It's better to miss printing the comment (or to report an error, as in #15158), but we should never generate invalid code.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 8, 2022

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

@liuxingbaoyu
Copy link
Member

This PR looks good to me, can we keep https://github.com/babel/babel/pull/15159/files#diff-6c32c344bfe1f4e77de24cdd3e427e84c4ac0a871380ff3720a7351cac63cb74R197-R198?
In my opinion this is better than being trailing Commens.

@liuxingbaoyu

This comment was marked as outdated.

@nicolo-ribaudo
Copy link
Member Author

Well, we could still keep the new this.printInnerComments(); call

@liuxingbaoyu
Copy link
Member

The current this._noLineTerminator is automatically set to false, which I fear does not completely prevent all line breaks.

Also the output in #15161 is weird, it seems like we are wrapping comments for non-existent, it might be worth me to open a PR to optimize.

@nicolo-ribaudo
Copy link
Member Author

The current this._noLineTerminator is automatically set to false, which I fear does not completely prevent all line breaks.

It's automatically set to false after printing a token/word. [no LineTerminator here] is only between two tokens, so after printing the second one is always safe to set _noLineTerminator to false.

@nicolo-ribaudo nicolo-ribaudo merged commit 4285d5f into babel:main Nov 8, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the hotfix branch November 8, 2022 16:45
@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 Feb 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: comments i: regression 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 PR: Fixes failing main
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: With comments: false, babel inserts an extra newline before the => of an arrow function
4 participants