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

repl: allow `await` in REPL #13209

Closed
benjamingr opened this Issue May 24, 2017 · 77 comments

Comments

Projects
None yet
@benjamingr
Copy link
Member

benjamingr commented May 24, 2017

I've seen this requested a few times in StackOverflow now:

People have code that is asynchronous and would like to use values from it in the REPL:

// how do I access in the REPL? returns promise
Transaction.where('reference', '1').fetch().then((res) => { return res }); 

A common workaround is to store a global reference in the REPL, which is a little hacky and relies on timing, but can work for simple cases:

> Transaction.where('reference', '1').fetch().then((res) => out = res)
[Object Promise]
> out
   /* response available here*/

This works, but has timing issues and generally isn't great. We could however run the REPL in the context of an async function potentially - which would allow awaiting values in the REPL:

let res = await Transaction.where('reference', '1').fetch(); 

I think it could be an interesting enhancement but would like more feedback on the idea. @nodejs/collaborators

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented May 24, 2017

👍

I looked into implementing this when Node 7.6.0 came out and async functions were originally added. At first glance it seemed like it would be a bit difficult because the REPL currently doesn't run in a function context at all. However, there might be a better solution that I'm not aware of.

@refack

This comment has been minimized.

Copy link
Member

refack commented May 24, 2017

I'm +1.
But there are userland solutions, quick googling found https://github.com/skyrising/await-repl, https://github.com/StreetStrider/repl.js, and I'm sure there are more

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented May 24, 2017

await-repl seems to only await a promise if the line starts with "await ", which is probably insufficient (e.g. it doesn't allow foo = await bar().

repl.js seems to auto-await all Promises that are returned from an eval, but that also probably isn't what we want (and it doesn't allow for nested await either).

@refack

This comment has been minimized.

Copy link
Member

refack commented May 24, 2017

I'm not arguing that this is a beneficial feature. But IMHO the REPL is an exploratory tool, so those tools at least enable easier exploration 🤷‍♂️
On the other hand the cli -e option, again IMHO, is a more "important" feature to enable easy await usage for.
node -e "await new Promise.resolve(4)"

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented May 24, 2017

The question is, is it really possible? It seems like it's not

@refack refack added the cli label May 24, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented May 24, 2017

It would definitely be possible in theory by reimplementing await semantics and parsing, but that seems like a lot of work. Ideally there would be some --allow-top-level-await flag in V8 that we can use.

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented May 25, 2017

There was a feature request issue for this and there are some solutions proposed there: #8382 although it seems most of them are somewhat hacky

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented May 25, 2017

@vkurchatkin

The question is, is it really possible? It seems like it's not

Why can't we just wrap the content with an async function?

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented May 25, 2017

@benjamingr, that will change the semantics of repl. We need to detect it somehow, or use a flag, or force this as a semver-major change for everyone.

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented May 25, 2017

@ChALkeR what semantics would it change? I figured that since the REPL is not synchronous anyway (being user input and all) the microtick of wrapping it in an async function - unwrapping it and giving the user back the result wouldn't be too bad.

Am I missing anything?

@targos

This comment has been minimized.

Copy link
Member

targos commented May 25, 2017

@benjamingr do you have ideas about how to make this work:

> const a = Promise.resolve(42);
undefined
> const b = await a;
undefined
> b
42

If we wrapped the code in an async function, b would be local to this function. How to unwrap it?

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented May 25, 2017

@targos you're absolutely right! I totally missed the scoping issue.

Parsing the line and figuring out if it's a variable declaration seems easier than detecting an async function, it would still be problematic for things like f(); const a = 5; which are legit.

I'm wondering if we could have an escape hatch at the V8 level perhaps? Optimally something that lets V8 know it should evaluate the code passed as an async function?

Essentially, we'd need a way for Script to take an async function (well, the Script::Compile method).

I'm wondering if that's possible - @nodejs/v8 @fhinkel ?

@bergus

This comment has been minimized.

Copy link

bergus commented May 26, 2017

This also needs some kind of cancellation option - like Ctrl+C - to stop waiting. Something like

await new Promise(() => {}) // forever pending

should not break the REPL

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented May 29, 2017

This seems like an odd idea to me, what does the repl do during an await? Does it just stall and not respond to further user input? How do you know when the await is completed otherwise?

Either way seems confusing and poor design to me, maybe I'm not considering something?

I suppose it's not much different that running a long sync operation although that is probably less common.

@Qard

This comment has been minimized.

Copy link
Member

Qard commented May 29, 2017

Yeah, I think stalling is the desired behaviour. The idea is to be able to inspect the return values of a promise API and use them for subsequent operations in a more friendly way. Currently you have to attach then() handlers to write values into global s and check repeatedly to see when they become available. It's very unfriendly to promise users.

@refack

This comment has been minimized.

Copy link
Member

refack commented May 29, 2017

Does it just stall and not respond to further user input? How do you know when the await is completed otherwise?

Maybe show a spinner?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented May 29, 2017

Does it just stall and not respond to further user input? How do you know when the await is completed otherwise?

Maybe show a spinner?

Sounds good. As long as you can see that something is happening and you can abort it with Ctrl+C, we should be good. :)

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented May 29, 2017

Maybe show a spinner?

Sounds inappropriate for REPL. Aborting is a must, though. That said, I think it's a waste of time to discuss such details until we figure out a way to do this.

@bmeck

This comment has been minimized.

Copy link
Member

bmeck commented May 30, 2017

at some point people might want to think of foreground and backgrounding await if you need to test how 2 async things interact... like Jobs in shells

@refack

This comment has been minimized.

Copy link
Member

refack commented May 30, 2017

foreground and backgrounding await

Isn't there a key for that in bash? CTRL-Z? than also jobs and bg and fg? (not a bash expert)
We already have .* REPL commands, so .jobs / .fg / .bg doesn't sound absurd...

@RReverser

This comment has been minimized.

Copy link
Member

RReverser commented May 30, 2017

It would definitely be possible in theory by reimplementing await semantics and parsing, but that seems like a lot of work.

FWIW that's what regenerator already did (as used by Babel for transpiling generators and async-await to ES5).

So for a problematic example above, wrapped into an async function:

(async function() { 
 const b = await a;
})();

it would generate

(function _callee() {
  var b;
  return regeneratorRuntime.async(function _callee$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
          _context.next = 2;
          return regeneratorRuntime.awrap(a);

        case 2:
          b = _context.sent;

        case 3:
        case "end":
          return _context.stop();
      }
    }
  }, null, this);
})();

From here, we could easily unwrap the IIFE and execute body, which would solve the scope propagation issue as all the modified variables would be visible after the body is executed.

However I'm not sure if we would want to integrate such, even minimal, third-party transpiler into Node.js REPL.

But if we do, I could try to make this happen :)

@chicoxyzzy

This comment has been minimized.

Copy link

chicoxyzzy commented May 30, 2017

IMO REPL should be predictable. await is valid identifier outside of async function. So it should work as identifier.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented May 30, 2017

However I'm not sure if we would want to integrate such, even minimal, third-party transpiler into Node.js REPL.

If you can make this happen and the code doesn’t make the node binary turn huge, I would be in favour of doing that.

@ebraminio

This comment has been minimized.

Copy link
Member

ebraminio commented May 30, 2017

I think you should consult some of v8 guys for help and taking best strategy here as the same feature would be nice for Chrome console as well so a similar approach could be used for node.js I guess.

(BTW I am very happy that this is proposed here as the idea of top-level await itself seems is rejected but all I personally liked to have was a console one)

@ebraminio

This comment has been minimized.

Copy link
Member

ebraminio commented May 30, 2017

Filed as http://crbug.com/727924 Edited: already available on http://crbug.com/658558

@ebraminio

This comment has been minimized.

Copy link
Member

ebraminio commented May 30, 2017

According to this comment and my test, this is already supported on Safari so I guess Chrome team should/will soonish pick up the task to fill the gap up with the other browser.

TimothyGu added a commit to TimothyGu/node that referenced this issue Sep 25, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Sep 28, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 22, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 22, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 22, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Oct 22, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

TimothyGu added a commit to TimothyGu/node that referenced this issue Nov 13, 2017

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Fixes: nodejs#13209

@TimothyGu TimothyGu closed this in eeab7bc Nov 16, 2017

@mercmobily

This comment has been minimized.

Copy link

mercmobily commented Dec 20, 2017

I just read the whole thread and figured out that this only landed 9 days ago...
So, which version of node can I expect this gem of a commit to appear?
Thanks for implementing this BTW. Much needed!

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Dec 20, 2017

@mercmobily It landed on Nov 16. The one 9 days ago is in a fork. It will definitely be in v10.0.0, but needs manual backporting to v9.x per #15566 (comment), which I have not been able to do :/

@mercmobily

This comment has been minimized.

Copy link

mercmobily commented Dec 20, 2017

I am on 8.x, so I assume it will never make it back here... which is OK. Thank you so much for the amazing work on it... async/await are the best things since sliced bread!

@gibfahn

This comment has been minimized.

Copy link
Member

gibfahn commented Dec 20, 2017

@mercmobily FYI, the relevant bit is this:

image

It should make it back to v8.x, although it will probably take a couple of months at least. You can subscribe to #15566 to get updates on backporting.

@mercmobily

This comment has been minimized.

Copy link

mercmobily commented Dec 21, 2017

Well actually just one hour ago bmeck is talking about reverting it...!

#15566 (comment)

@chicoxyzzy

This comment has been minimized.

Copy link

chicoxyzzy commented Dec 21, 2017

This is actually good news. Behavior should be predictable

msoechting added a commit to hpicgs/node that referenced this issue Feb 7, 2018

repl: support top-level await
Much of the AST visitor code was ported from Chrome DevTools code
written by Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>.

PR-URL: nodejs#15566
Fixes: nodejs#13209
Refs: https://chromium.googlesource.com/chromium/src/+/e8111c396fef38da6654093433b4be93bed01dce
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@terrywh

This comment has been minimized.

Copy link

terrywh commented Mar 13, 2018

any news on this ?

@Krinkle

This comment has been minimized.

Copy link
Contributor

Krinkle commented May 2, 2018

For the record:

  1. Yes, this was landed in master in December 2017, and has been released in Node 10.
  2. But, it has since been put behind a CLI flag: --experimental-repl-await, per #19604.

Node 10 with --experimental-repl-await provides a working REPL, today, that supports await!

🎉

@esteban-uo

This comment has been minimized.

Copy link

esteban-uo commented May 15, 2018

thanks for supporting it!. I don't know if it is just me, but in Node 10.1, it just hangs out while using REPL await... anyone else experiencing the same?

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented May 15, 2018

@esteban-uo It seems you need --experimental-repl-await for this now.

@esteban-uo

This comment has been minimized.

Copy link

esteban-uo commented May 15, 2018

@vsemozhetbyt yes and actually it works just fine 👍 Seems to be it happens when an exception occurs, which is very confusing sometimes, since you could think that the promise can't be resolved at all or a timeout is happening

@domainoverflow

This comment has been minimized.

Copy link

domainoverflow commented May 29, 2018

Forgive me if I am misreading but according to the above I would be able to CLI node app.js where app.js could contain awaitS placed outside functions ( ex: copy and paste from puppeteersandbox.com examples without wrapping and cli running it ) ?

@Krinkle

This comment has been minimized.

Copy link
Contributor

Krinkle commented May 29, 2018

@domainoverflow Thank you for asking!

This issue is only about using "await" in the special "REPL" mode of Node.js. If you run node as CLI command without JS file, that is the REPL mode. This mode is similar to the web browser DevTools console and mainly for testing things.

REPL is a different from how programs run normally. And for JS programs, it is not possible to use await outside async functions because the JavaScript programming language is specified in a way that does not allow this. There is a proposal from TC39 to allow "await" outside functions in normal programs. More about that at https://github.com/tc39/proposal-top-level-await. This was relatively easy to do in REPL for debugging and testing, but is less easy for normal programs. If the proposal is approved, then the JavaScript engine for Node.js (Google V8) may implement the change, after which it may eventually become available in Node.js.

@techsin

This comment has been minimized.

Copy link

techsin commented Sep 4, 2018

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment