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

ES7 async/await implementation with concise syntax, plus tests #978

Closed
wants to merge 9 commits into from

Conversation

gkovacs
Copy link
Contributor

@gkovacs gkovacs commented Sep 20, 2017

Based on #836 which unfortunately wasn't merged because it lacked tests and the original author (@summivox) asked for it to be taken over, please see there for background and discussion.

I have added tests in this pull request, I have been using this in using this in production for 3 months without issue so I am fairly certain it is correct code-wise but let me know if there are any additional tests that should be added.

It appears the documentation for this project is maintained in a separate branch (gh-pages, I believe) so I will make a pull request to that branch adding documentation for this feature once this pull request is merged.

summivox and others added 5 commits March 30, 2017 03:07
see http://wiki.ecmascript.org/doku.php?id=strawman:async_functions

async function: `->>`, `~>>`, `-->>`, `~~>>`, `async (arglist) ->...`, `async function name (arglist)`
await: `await` => `await`; `await all` => `await*` (proposed to translate to `Promise.all`, not final)

backcall does NOT work, but this should be fine because they were invented to create the illusion of async function anyway
@gkovacs
Copy link
Contributor Author

gkovacs commented Sep 20, 2017

Note that because only nodejs 7 and 8 support the async/await syntax, the tests will only run on nodejs 7 and 8, hence I've adjusted scripts/test accordingly to make travis-cl happy (as .travis.yml still specifies that it should be testing on nodejs 4 and 6).

Copy link
Collaborator

@rhendric rhendric left a comment

Choose a reason for hiding this comment

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

@gkovacs, thank you for taking this over! This should be ready for a merge after these minor points are addressed.

src/lexer.ls Outdated
@@ -240,6 +240,9 @@ exports <<<
or last.1 is 'import' and 'All'
last.1 += that
return 3
if last.0 is 'yield' and last.1 is 'await'
last.1 += 'all'
return 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see support for awaitall tokens anywhere else; is this block needed? (If so, there should be an await yield all test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed, removed it

src/ast.ls Outdated
| 'yield' => ''
| 'yieldfrom' => 'from'
| 'await' => 'await'
| _ => ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove the default case if it has no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/async.ls Outdated
x = ->>
3

y <- x!then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you wrap the backcall tests in their own blocks so that they don't subsume the rest of the file? As it is, if x (somehow) threw an error, then I believe this file would silently pass without running any further tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(By ‘threw an error’ I of course mean ‘produced a Promise that doesn't get fulfilled’.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/async.ls Outdated

y <- x!then

eq y, 16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add one test each for:

  • async function name(arglist) syntax
  • !->> (hushed async functions should still return Promises, but they shouldn't be implicitly fulfilled with the value of the body)
  • -->>
  • ~>>
  • a compile-throws test for ->>*

No need to do all the combinations (!~~>>), just a quick smoke test of each is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The ->>* results in a parse error which I didn't want to over-fit the test to (also I couldn't find a straightforward way to detect ->>* without complicating the grammar) so I added compile-parse-error method that looks for parse errors in scripts/test.

@gkovacs
Copy link
Contributor Author

gkovacs commented Sep 21, 2017

@rhendric OK removed the dead code and added those additional tests, should be ready for another round of reviews

rhendric added a commit that referenced this pull request Sep 21, 2017
@rhendric
Copy link
Collaborator

I manually merged to master, cleaning up the git history and making a few tweaks mostly to the testing. Thank you again!

@rhendric rhendric closed this Sep 21, 2017
@ymeine
Copy link

ymeine commented Sep 28, 2017

This is a great feature, finally! :)

I was starting to change my code, and there is still one thing I am missing though: async backcalls. For now it's trying to parse it as function composition. This is mainly to simulate top-level await (in a more controlled way), but it could also be useful when using any function receiving an async callback.

Is there any plan to implement the async backcall syntax?

@rhendric
Copy link
Collaborator

@ymeine, I believe that the original thinking around leaving them out was that backcalls are the poor man's version of await, and mixing both syntax features would be unnecessarily confusing and shouldn't be encouraged. I don't think they'd be hard to add, but I'd like to see more of an argument for why they're needed (instead of promisifying any callback-based functions and only using await instead of backcalls).

@ymeine
Copy link

ymeine commented Oct 1, 2017

Well, as I said there are the top level await: after importing all my modules, I execute the rest of my module in an async function so that I can write asynchronous code in synchronous style, using await. This is mainly done for top-level modules I must admit it.

Otherwise, the case of a "callback" as I named it, if it's a true callback I understand and agree it's better to promisify them, but if it's let's say a simple argument which turns out to be an asynchronous function, in some cases if it's the last call of the enclosing function we might want to use a backcall. But I would say it would be confusing and quite a rare case.

Therefore I will say it's mainly for top-level await.

In my scripts, I often do that for now:

*<- co
# ...
yield my_async_function!
# ...

and it would be nice to be able to use an async function instead, without having to indent my whole code afterwards:

<<- call_it
# ...
await my_async_function!
# ...

(well it's still a pain to have an immediately executing backcall or I missed something in LiveScript capabilities)

@rhendric
Copy link
Collaborator

rhendric commented Oct 1, 2017

If what you really want is top-level await, I think I'd rather try to give you top-level await. LiveScript already compiles everything inside an IIFE by default; I can look into what it would take to make that IIFE async if there are any awaits outside of any functions. (It would be a compile error to try to use top-level await with the --bare compilation option.) How does that sound?

@ymeine
Copy link

ymeine commented Oct 1, 2017

I think that's a good trade-off, it would cover my main use case, while still being a non-intrusive feature (applied only if there is a top-level await) and taking advantage of an already present feature (the wrapping IIFE). Moreover it would go beyond and solve my issue which was the auto-invocation of the backcall.
To me that sounds great!

@bartosz-m
Copy link
Contributor

@ymeine You could try two packages that I've coded today:
transform-top-level-await it allows to write

foo = Promise.resolve \foo
bar = Promise.resolve \bar 
foo-bar = ->> "#{await foo} #{await bar}" 
x = await foo-bar!

add transform-implicit-async and you can omit ->> and just write ->

foo = Promise.resolve \foo
bar = Promise.resolve \bar 
foo-bar = -> "#{await foo} #{await bar}" 
x = await foo-bar!

@rhendric rhendric mentioned this pull request Mar 17, 2018
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.

5 participants