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 single-arg async arrows when retainLines=true #8868

Merged
merged 1 commit into from Oct 30, 2018
Merged

fix single-arg async arrows when retainLines=true #8868

merged 1 commit into from Oct 30, 2018

Conversation

ryan-mars
Copy link
Contributor

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

I came across #7275 trying to work around facebook/create-react-app#5319 @roblourens pointed me here.

re: @loganfsmyth

var fn = async (
  arg
) => {}

with retainLines: true becomes

var fn = async 
  arg => {}

which is a syntax error since async and arg need to be on the same line when there aren't parens.

We need to detect that retainLines is enabled, and that the identifier will insert new lines, and if so, wrap parens around the argument.

So that's what I did but there's only one new test. This could probably use a few more test cases but I don't know what they would be, I just want the upstream issues (facebook/create-react-app#5319, jestjs/jest#5326, microsoft/vscode#60468, microsoft/vscode#60187) to be resolved so I can go on my merry way and this seemed to be the root blocker. If someone could point me to a few more cases that this should handle I will gladly add them.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 13, 2018

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

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.

Can you also test this?

async (x)
=> {}

Btw, I really hope we can remove the retainLines option and just use sourcemaps in the future.

@ryan-mars
Copy link
Contributor Author

ryan-mars commented Oct 13, 2018

@nicolo-ribaudo Sure thing. What should it output with retainLines?

async (x) =>
{};

Chrome throws Unexpected token => for

async (x)
=> {}

Also, yes I recognize everyone wants to get rid of retainLines but many tools unfortunately still rely on it. 🙁

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 13, 2018

Ok, I give you a bad example 😅
It was just to check that it works even if the body is in the first line after the parameters

@ryan-mars
Copy link
Contributor Author

Added it anyway. Is this safe to merge?

@existentialism existentialism added pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Oct 14, 2018
@ryan-mars
Copy link
Contributor Author

Should I abandon this?

@nicolo-ribaudo
Copy link
Member

Nope 🙂

@nicolo-ribaudo nicolo-ribaudo merged commit de80aef into babel:master Oct 30, 2018
@ryan-mars
Copy link
Contributor Author

My first commit to Babel!!! 🎉

mAAdhaTTah added a commit to valtech-nyc/babel that referenced this pull request Nov 3, 2018
* master: (222 commits)
  Set correct methods name
  Use toPropertyKey in the "decorate" helper
  Allow function types in type params within arrow return types (babel#8954)
  Fix message when plugin of a wrong type is passed (babel#8950)
  rename colliding let bindings with for loop init (babel#8937)
  edge incomplete support for arrow destructuring (babel babel#8349) (babel#8926)
  fix single-arg async arrows when retainLines=true (babel#8868)
  [flow] Explicit inexact objects with `...` (babel#8884)
  Update preset-env data (babel#8898)
  Treat break inside block inside loop (babel#8914)
  fixed "source map" formatting in comment (babel#8878) [skip ci]
  fix typo in contributing guidelines (babel#8901) [skip ci]
  fix: Expression x === 'y' && '' should not evaluate to undefined. (babel#8880)
  fixed an extra word
  Fixes babel#8865 (babel#8866)
  v7.1.4
  v7.1.3
  Bump Babel deps (babel#8770)
  flow-bin@0.82.0 (babel#8832)
  Insertafter jsx fix (babel#8833)
  ...

# Conflicts:
#	packages/babel-parser/src/tokenizer/index.js
#	packages/babel-parser/test/fixtures/experimental/class-private-properties/failure-numeric-literal/options.json
#	packages/babel-parser/test/fixtures/experimental/pipeline-operator/invalid-proposal/options.json
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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.

retainLines breaks no LineTerminator requirements in single-arg async arrows and decorated methods
4 participants