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

Enable await in expressions #11

Closed
wants to merge 1 commit into from

Conversation

vmarkovtsev
Copy link

@vmarkovtsev vmarkovtsev commented Aug 26, 2018

The implementation is copied from Node's --experimental-repl-await

Screenshot of ijavascript with this patch:

screenshot from 2018-08-27 00-50-34

DISCLAIMER: I am a totally lame JS programmer πŸ€¦β€β™‚οΈ: I have to spend some time to figure out how to write tests for this project, how to properly integrate acorn - it is currently integrated the same way as in Node, etc. So consider this a PoC. Feel free to implement everything by yourself and close this PR (I mean, really) or bear with me to make a proper PR. In the latter case I will be grateful for your JS improvement hints and lessons πŸ˜„

The implementation is copied from Node's --experimental-repl-await

Signed-off-by: Vadim Markovtsev <vadim@sourced.tech>
@vmarkovtsev
Copy link
Author

  • Read CONTRIBUTING.md
  • Added await tests
  • Added the ability to skip tests based on the node version (prior to 7.6 do not have await)
  • Passed CI

await.js is copied so I excluded it from eslint.

@alancnet
Copy link

alancnet commented Sep 7, 2018

+1 on this. Hope to see this merged, and mine closed.

@n-riesco
Copy link
Owner

n-riesco commented Sep 7, 2018

@vmarkovtsev @alancnet Sorry for the silence. I've had a quick read, but I haven't had a chance to test this PR.


I've just had an idea while thinking about this PR: I'd like to test if it's possible to replace this PR with a PR that adds the flag --experimental-repl-await to this line.

@vmarkovtsev
Copy link
Author

@n-riesco I've already tried that but it did not work for me. The reason is that this flag works only in the REPL mode.

@vmarkovtsev
Copy link
Author

ping

@n-riesco
Copy link
Owner

@vmarkovtsev apologies for the silence. I'll work on this tomorrow.

n-riesco added a commit to n-riesco/ijavascript that referenced this pull request Sep 20, 2018
@n-riesco
Copy link
Owner

@vmarkovtsev I've moved this PR to a branch of the IJavascript repository. For the time being, I'm distributing this branch in the package ijavascript-await (I did something similar with jmp and jmp-prebuilt when we were testing the transition from zmq to the prebuilt binaries of zeromq).

I'm also distributing lib/server/await.js as package jp-parser. The main reason for this is to keep NEL free of external dependencies.

Ideally, I'd like this functionality to be part of Node.js (I'll explore what is needed at some point in the future). I don't want to go down the route where IJavascript reimplements Node.js.


@vmarkovtsev I'd like to give you credit for your contribution here. Please, would you have a look at the branch I created in the IJavascript repository? And if you're happy, please would create a PR against that branch to update the file AUTHORS with your details?

@vmarkovtsev
Copy link
Author

@n-riesco Sure, as long as it works I am fine πŸ˜„ n-riesco/ijavascript#172

n-riesco added a commit to n-riesco/ijavascript that referenced this pull request Sep 20, 2018
* kernel: Enable await in global expressions (thanks to Vadim
  Markovtsev. See n-riesco/nel#11 .

Closes n-riesco/nel#11
Closes n-riesco/nel#10
@n-riesco
Copy link
Owner

@vmarkovtsev Your tests are passing πŸ˜‰!

@alancnet

For anyone wanting to test IJavascript with global await expressions, please, note that for the time being I'm distributing this feature in the packageijavascript-await:

npm install -g ijavascript-await

@n-riesco n-riesco closed this Sep 20, 2018
@vmarkovtsev
Copy link
Author

Thanks!

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