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

Add options.allowAwaitOutsideFunction. #7637

Merged
merged 1 commit into from
Mar 29, 2018
Merged

Add options.allowAwaitOutsideFunction. #7637

merged 1 commit into from
Mar 29, 2018

Conversation

jdalton
Copy link
Member

@jdalton jdalton commented Mar 27, 2018

This PR addresses #7634.

It's missing tests as the moment. (I'll rebase them in soon)

Update:

✅ Added tests.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 27, 2018

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

@babel-bot
Copy link
Collaborator

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

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 27, 2018

WDYT of naming it allowTopLevelAwait, since it is how it's usually referred to?

Also, I think that this code should still throw:

function fn() {
  await x;
}

@jdalton
Copy link
Member Author

jdalton commented Mar 27, 2018

Fixed lint errors, added tests, and ensured an error is thrown for the case raised by @nicolo-ribaudo.
I kept the name as allowAwaitOutsideFunction since it's technically more correct and aligns with existing and similar options (feel free to tweak to taste however).

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

thanks for your.

Could we maybe rename the tests? core/uncategorised/555 is not super obvious.

@jdalton
Copy link
Member Author

jdalton commented Mar 28, 2018

Could we maybe rename the tests?

I wasn't sure about that but noticed a large collection of tests incremented as numbers. Is there a particular place or naming scheme you had in mind?

@nicolo-ribaudo
Copy link
Member

You could put it in fixtures/es2017/async-functions/allowAwaitOutsideFunction.

I kept the name as allowAwaitOutsideFunction since it's technically more correct and aligns with existing and similar options

I still prefer the allowTopLevelAwait name, but I'm ok with this since it is how allowReturnOutsideFunction is named.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 28, 2018

I just learned that this is actually a new proposal (https://github.com/MylesBorins/proposal-top-level-await). Can you define this feture it using a new Babylon plugin (topLevelAwait) instead of an option?

Something like this:

if (isAwait && !this.state.inFunction) this.expectPlugin("topLevelAwait");

The tests would then go in packages/babylon/test/fixtures/experimental/top-level-await/basic/input.js. Also, the Babylon readme should then be updated.

@jdalton
Copy link
Member Author

jdalton commented Mar 28, 2018

I rather not since the proposal, and its variations, are much more involved. I think later, if someone wants to add a plugin to support the proposal (and variations) more in-depth, then they could build on top of allowAwaitOutsideFunction

Update:

Moved tests to a more readable place.

@hzoo
Copy link
Member

hzoo commented Mar 29, 2018

I think later, if someone wants to add a plugin to support the proposal (and variations) more in-depth, then they could build on top of allowAwaitOutsideFunction

that's true

@hzoo hzoo merged commit 59ba395 into babel:master Mar 29, 2018
@jdalton jdalton deleted the allowAwaitOutsideFunction branch March 29, 2018 14:47
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: enhancement outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants