GeezerVison

Can you write a small example where let and const are failing?

Great work! This is the right approach IMO.

Is everything with acorn OK license wise?

@Inf3rno

Can you write a small example where let and const are failing?

Sure.

const a = await Promise.resolve(1);

Enter

a = 3;

Expected: TypeError: Assignment to constant variable.

Actual: 3

@benjamingr Acorn uses the MIT License so our licenses are compatible. I've also added Acorn to tools/license-builder.sh and the root-level LICENSE file.

I mean, Node’s REPL already has its problems with top-level scope const and let … if we’re starting to transform anyway, is replacing those a viable option?

(Btw, I would like it if the REPL just allowed overwriting const variables …)

@addaleax

Node’s REPL already has its problems with top-level scope const and let …

That is certainly true. (e.g. #13919)

if we’re starting to transform anyway, is replacing those a viable option?

(Btw, I would like it if the REPL just allowed overwriting const variables …)

Technically yes, but I'm not convinced yet that's a good idea as it goes against the keywords' intended semantics, and what Chrome is doing. It sounds like a slippery slope.

+1 for allowing writing over const variables

REPL is an environment meant for experimentation. I feel like true const-ness actually doesn't really make much sense there anyway.

@TimothyGu

Is it transformed to the following?

// const a = await Promise.resolve(1);
// a = 3

(async () => { void (a = await Promise.resolve(1)) })()
a = 3

If every await gets its own asnyc function wrapper, then I don't think we can do much about it. Still I think it is worth breaking this const feature. Maybe we should allow only var by await and send an error message if somebody tries to use const or let with await.

I am not that experienced with this REPL project, but if this

var a = await Promise.resolve(1);
console.log(a); // 1
a = 3;
console.log(a); // 3

is transformed into this:

(async () => { void (a = await Promise.resolve(1)) })()
// console.log(a); // ReferenceError - a is not declared

a = 3
console.log(a); // 3

// setTimeout(() => {
//	console.log(a); // 1
// }, 10);

then I think we have a problem, because I'd expect this behavior:

(async () => {
	var a = await Promise.resolve(1);
	console.log(a); // 1
	
	a = 3;
	console.log(a); // 3
})()

To make it work this way we need to change the REPL state to pending until the promise is fulfilled and continue with the next line only after that. I am not sure whether that would be an acceptable behavior. Another possible solution would be to display a message if the promise is fulfilled and you can use the results. If you choose that one, then it might be beneficial to prevent the user from setting a new value to the variable before it gets its value from the promise. Maybe I am misunderstanding something, because I don't have a testing environment to check your commit and tbh. I don't fully understand it, but this might be a harder problem than just wrapping the actual line.

@TimothyGu Behaviour is difference in canary.

@princejwesley

Behaviour is difference in canary.

I'm sorry, I don't quite understand what you mean. The code you put in Canary's DevTools and the output you got is the exact same as what you would get from this PR.

> const b = await 4
undefined
> b
4
> const c = await Promise.resolve(1)
undefined
> c
1
> c = 4
4
> c
4

@TimothyGu
can we translate var|const|let a = await func(); into

(async (self) => {
  var $ret = await func();

  // const case
  Object.defineProperty( self, 'a', {
      get: function () {
          return $ret;
      },
      set: function() {
          throw new TypeError('Assignment to constant variable')
      }
  });

  // let re-declaring case
  // if self has 'a', throw SyntaxError
  Object.keys(self).findIndex((key) => key === 'a') !== -1 &&  throw new SyntaxError("Identifier 'a' has already been declared");

  // let and var case

  self['a'] = $ret;

  void 0;

})(global);

?

@Inf3rno

I am not that experienced with this REPL project, but if this

var a = await Promise.resolve(1);
console.log(a); // 1
a = 3;
console.log(a); // 3

is transformed into this:

(async () => { void (a = await Promise.resolve(1)) })()
// console.log(a); // ReferenceError - a is not declared

a = 3
console.log(a); // 3

// setTimeout(() => {
//	console.log(a); // 1
// }, 10);

then I think we have a problem

No it doesn't actually transform to that. After you enter the first line, the REPL will PAUSE until the promise returned from the async function is resolved. So when you can enter the next line, a has to be defined already.

> var a = await Promise.resolve(1);
undefined
> console.log(a)
1
undefined
> a = 3
3
> console.log(a)
3
undefined

Again, I encourage y'all to actually build this version and test it out. I'm fine with answering questions, but the fastest way to answer those questions are to find out about them yourselves 😘

@princejwesley No, because that would break cases like

var a = 0
let a = await 0

@princejwesley let and const are not supposed to influence the global object at all.

@TimothyGu I am just trying to mimic the let/const/var behaviour with the context.

@princejwesley I got you. While there are indeed some creative ways to do that, seeing how people seem to not like the restrictions of let and const at all in REPL (1/2/3) I don't think we should try to make it worse ;)

@TimothyGu Okay, thanks!

I'm in favor of this feature, but am also +1 on reverting until this is better sorted out.

If we do decide to go the revert route, here is a PR: #17807

we don't land it in a release with odd behavior we are stuck with

There are no REPL implications we would be “stuck” with, since any fix to REPL would be a semver-patch change.

The special async mode is certainly not ideal, but the way we are operating regarding source code transformation is identical to how DevTools is; and it is my strong belief that if something is good enough for the most widely used browser in the world, it should be good enough for Node.js (not saying it couldn’t be better).

I’m fine with putting this behind a flag, but given the positive developer feedback we have received so far, and the strictly additive nature of this feature (it doesn’t break any existing use cases) I am against reverting this wholesale.

The special async mode is certainly not ideal, but the way we are operating regarding source code transformation is identical to how DevTools is; and it is my strong belief that if something is good enough for the most widely used browser in the world, it should be good enough for Node.js (not saying it couldn’t be better).

V8 also have an open issue on moving to a more native/feature safe without code transform to do this. Given that they are seeking to change things as well, I'm not swayed that because it is broken other places it should be ok to be broken all the places.

I’m fine with putting this behind a flag, but given the positive developer feedback we have received so far, and the strictly additive nature of this feature (it doesn’t break any existing use cases) I am against reverting this wholesale.

That seems fine, I wouldn't want it unflagged until stuff is ironed out. Some of the behavior does need to be specified like how this mode made const not const, allowed variable re-binding, and is using the global object instead of scope.

Now that I see how this is done, it looks like this could easily be done in userland with Node 8, right?

@jedwards1211 it is non-trivial and I am having to patch internals on node's repl to fix various things. I think it is unlikely to be able to be done completely in userland without something very large in scope that reimplements a large part of the repl.

@jedwards1211

Now that I see how this is done, it looks like this could easily be done in userland with Node 8, right?

Go for it! I'd love to see what userland comes up with 🙌

@bmeck The following hack works okay in my personal scripts. Obviously it's not production quality:

    if (/\bawait\b/.test(cmd) && !/\basync\b/.test(cmd)) {
      const match = /^\s*var\b(.*)/.exec(cmd)
      if (match) {
        cmd = `(async () => { void (${match[1]}) })()`
      } else {
        cmd = `(async () => { return ${cmd} })()`
      }
    }

What part of the repl are you having to reimplement?

@jedwards1211 if you enable such a transform to always be present repl.js most of the test suite for REPL fails. In the case of the specific above it has problems for multiple statement commands. In the case of the top level await transform that landed various things are problematic: see comment above , rewriting the pause mechanic is pretty significant and making sure that none of the internals rely on synchronous behavior is a main reason why the test suite fails. The transform that landed side stepped the failures by splitting the REPL into two evaluation modes and did not run the existing tests against the new mode, running the transform in the test suite you can see lots of problems.

@bmeck I'm confused why a pause mechanic was necessary, since the eval option accepts a callback, which was sufficient for what I'm doing in userland.

@jedwards1211 there are 3 pause mechanics in repl

1. the readline interface.pause()

Which is managing interrupt events from TTY for the most part.

2. the readable stream.pause()

Which is managing backpressure for the most part.

3. the internal REPL pause system

Which is intended to queue lines while paused, but doesn't do so if you use APIs like replserver.eval or fire events like 'line' on the other 2 pausable mechanisms directly. It does not fully queue things during an asynchronous eval.

Edit To see more complete examples of this behavior not being fully paused in the current mechanics pipe into the repl using cat multi-line-file | node -i and mix await with other things. (irc log link)

@jedwards1211 sorry to bother you, I'd like to use your hack, but I don't know where this code should go?

@wmertens it goes within the eval function you pass when creating the repl