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

[CS2] Don’t require async/await support to run coffee #4679

Merged
merged 3 commits into from
Sep 1, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Sep 1, 2017

Aside from #4604, the coffee command runs in Node 6 (though the tests don’t). See #4676. I feel like we might as well support Node 6 if we can, and it seems that all we need to do so is convert one await into a Promise.

I also added a test for the functionality added in #4604, plus a guard so that the tests (other than the async ones) can also run in Node 6.

I noticed that cake test is returning before the async tests have completed; see #4680.

…t’ wrapper in the REPL to use a Promise instead of the ‘await’ keyword; add tests for REPL ‘await’ wrapper, including test to skip async tests if the runtime doesn’t support them
Cakefile Outdated
try
new Function('async () => {}')()
yes
catch exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

exception isn't used here so you could just as well remove it.

@@ -409,6 +409,17 @@ runTests = (CoffeeScript) ->
description: description if description?
source: fn.toString() if fn.toString?

global.supportsAsync = if global.testingBrowser
Copy link
Collaborator

@connec connec Sep 1, 2017

Choose a reason for hiding this comment

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

Do we really need this if? Could we not just always feature test it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not. See the comment at the top of test/async.coffee. Even with a feature test at the top of that file, and a return if it fails, a runtime that doesn't support async will throw an error just from trying to parse the file. Also there's an async test in repl.coffee now, so I want to feature test this once and cache the result rather than having it in two places.

Copy link
Collaborator

@connec connec left a comment

Choose a reason for hiding this comment

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

One request for clarification, but otherwise LGTM 👍

@connec
Copy link
Collaborator

connec commented Sep 1, 2017

Also, if we want to support node 6 should we update engines in package.json? https://github.com/jashkenas/coffeescript/blob/2/package.json#L14

@GeoffreyBooth
Copy link
Collaborator Author

I thought about updating the engines key, but I think we should either leave it at 7.6 or up it to 8, because otherwise we'll get people complaining that we don't really support Node 6 because we say that async is supported but if they try to use async in Node 6 (like in the REPL) it'll fail; I don't know, what do you think? I guess you could make the same argument about JSX.

@connec
Copy link
Collaborator

connec commented Sep 1, 2017

Yeh... this is a bit of a murky area. I'm not sure how many people really pay attention to the engines stuff anyway.

I feel like package.json should mandate a minimum version for coffee to be functional, and make sure people are aware that syntax such as await and yield (and JSX) in compiled (or REPL-interpreted) JS will depend on runtime support or further compilation (e.g. babel).

@GeoffreyBooth
Copy link
Collaborator Author

I just fear another #4610. But I guess if the tests pass, and coffee works, we should say we support Node 6+.

Maybe it’s time to set up Travis CI to run builds/tests in Node 6 and 8 on every push.

@GeoffreyBooth GeoffreyBooth merged commit 6714869 into jashkenas:2 Sep 1, 2017
@GeoffreyBooth GeoffreyBooth deleted the dont-require-await-support branch September 2, 2017 01:46
@GeoffreyBooth GeoffreyBooth mentioned this pull request Sep 2, 2017
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.

3 participants