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.

Sign up for GitHub

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

Jump to bottom

retainLines breaks no LineTerminator requirements in single-arg async arrows and decorated methods #7275

Closed
loganfsmyth opened this issue Jan 26, 2018 · 5 comments · Fixed by #8868
Closed

retainLines breaks no LineTerminator requirements in single-arg async arrows and decorated methods #7275

loganfsmyth opened this issue Jan 26, 2018 · 5 comments · Fixed by #8868
Labels
i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator

Comments

@loganfsmyth
Copy link
Member

loganfsmyth commented Jan 26, 2018 •
edited 

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. We've got some existing logic in place for noLineTerminator but I don't think it would apply very well to this case. :(

Edit: A second example of this issue can also be found in #8650, so if we fix this issue, we should fix that one at the same time.
@Jessidhia
Copy link
Member

The latest version of Jest is supposed to have source map support; maybe babel-jest can be fixed to use that instead and we can finally get rid of retainLines.

@loganfsmyth
Copy link
Member Author

Maybe. Basically all I ever knew was "something in Facebook relies on it". Was that specifically Jest? Maybe @kittens would know?

@julien-f
Copy link

julien-f commented Jan 26, 2018 •
edited

Hey, is it related to jestjs/jest#5326 ?

@SimenB
Copy link
Contributor

SimenB commented Feb 17, 2018

Opened up a PR in Jest removing retainLines: jestjs/jest#5594

I have no idea if other parts of FB requires it, though

facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this issue May 14, 2018
@hansonw @facebook-github-bot
Remove async/await and strict Babel transforms
079af14 
Summary:
- Node 7.9.x has a stable native async/await implementation, so we can drop those transforms.
  - Unfortunately `retainLines` has a bug where it incorrectly splits lines with `async`, so I had to disable it :( babel/babel#7275
- The "use strict" transform isn't necessary as it's automatically done by the inline-imports transform.

Reviewed By: zertosh, aaronabramov

Differential Revision: D7864751

fbshipit-source-id: 7bf92ef5397d563d4e646af8e29445ee4849a518
hansonw added a commit to facebookarchive/atom-ide-ui that referenced this issue May 14, 2018
@hansonw
Remove async/await and strict Babel transforms
8d6a91b 
Summary:
- Node 7.9.x has a stable native async/await implementation, so we can drop those transforms.
  - Unfortunately `retainLines` has a bug where it incorrectly splits lines with `async`, so I had to disable it :( babel/babel#7275
- The "use strict" transform isn't necessary as it's automatically done by the inline-imports transform.

Reviewed By: zertosh, aaronabramov

Differential Revision: D7864751

fbshipit-source-id: 7bf92ef5397d563d4e646af8e29445ee4849a518
@SimenB
Copy link
Contributor

SimenB commented May 29, 2018

Jest has now removed all its usage of retainLines. Most happened in Jest 22, while we cleaned the rest up for 23. So Jest's not a blocker for removing it anymore, at least

@loganfsmyth loganfsmyth mentioned this issue Sep 7, 2018
Closed
@loganfsmyth loganfsmyth changed the title retainLines breaks no LineTerminator requirements in single-arg async arrows retainLines breaks no LineTerminator requirements in single-arg async arrows and decorated methods Sep 7, 2018
@roblourens roblourens mentioned this issue Oct 10, 2018
Closed
@ryan-mars ryan-mars mentioned this issue Oct 13, 2018
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix single-arg async arrows when retainLines=true
5 participants
@Jessidhia @loganfsmyth @julien-f @SimenB @xtuc