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

Support top-level await #5371

Merged
merged 7 commits into from
Sep 15, 2021
Merged

Conversation

edemaine
Copy link
Contributor

@edemaine edemaine commented Sep 13, 2021

@GeoffreyBooth mentioned that CoffeeScript is missing support for top-level await. This PR adds that support.

Mostly this involves doing fewer checks for invalidity. The only hard part is when the code is wrapped in a top-level IFFE, where we need to detect whether that wrapper should be marked async.

Fixes #5354 (request from Deno side).

The REPL already supported top-level await by explicit wrapping in an async function closure. I think it's best to keep that code, because older versions of Node don't support top-level await.

Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! A few minor notes and then happy to land.

test/async.coffee Outdated Show resolved Hide resolved
test/async.coffee Outdated Show resolved Hide resolved
@edemaine
Copy link
Contributor Author

edemaine commented Sep 14, 2021

I've updated the tests to

  • use eqJS
  • extend eqJS to allow specifying compile options
  • label tests better as to what's being tested

Happy to make further changes.

Update: I just saw your comment saying to not modify eqJS. I'll change that back.

@edemaine
Copy link
Contributor Author

Further revised the tests to check what they actually care about, which is a startsWith prefix. eqJS is back to the original.

@GeoffreyBooth
Copy link
Collaborator

Thanks. I want to leave this open a little bit in case anyone else wants to review (@helixbass?) and then will land and cut a new release.

I added #5372 to add Node 16 support to CI but the tests fail there, so that should also probably be addressed before we land this. cc @helixbass on that too as it looks like it has to do with AST stuff.

@GeoffreyBooth
Copy link
Collaborator

Got CI fixed. Can you please rebase off of master or merge master into your branch? That should trigger a new test run that’ll include Node 16.

FYI besides rebuilding the regular compiler we also need to build the browser compiler (cake build:browser). We should add a CI step that rebuilds everything and then checks that there’s nothing different from the last git commit (which, if there was a difference, would mean that the author forgot to rebuild and commit one of the outputs).

@GeoffreyBooth GeoffreyBooth merged commit c4f1fe7 into jashkenas:master Sep 15, 2021
@GeoffreyBooth
Copy link
Collaborator

Thanks. I’ll cut a new release branch soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deno can await outside function, how to remove " error: await can only occur inside functions "
2 participants