Fix _step.value access in for await

[nicolo-ribaudo]

Q	A
Fixed Issues?	Fixes #13490
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

The relevant spec steps are 6.d-6.f of 14.7.5.7 ForIn/OfBodyEvaluation.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47012/

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0927c7e:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[nicolo-ribaudo]

It seems like the compiled code works in Node.js >= 12 but not in Node.js 10 🤔 Maybe it's because of tc39/ecma262#1250.

[nicolo-ribaudo]

We should write a note in the docs that if someone cares about the exact number of ticks in await they need to make sure to enable transform-async-to-generator.

[danieltroger]

We should write a note in the docs that if someone cares about the exact number of ticks in await they need to make sure to enable transform-async-to-generator.

Do you mean that this PR will be hidden behind some flag? If so I'm against it. I'd like my code to work without having to google for 30 minutes and figuring out I need to enable a flag. That's exactly what made me stop using tsc.

[nicolo-ribaudo]

No, this PR will be enabled by default. However, there are still some edge cases that are not handled.

Consider this code:

Promise.resolve()
  .then(() => console.log(1))
  .then(() => console.log(2))
  .then(() => console.log(3));

(async function () {
  console.log(await { then: f => f("A") });
  console.log("B");
})()

Both in Node.js 16 and in Node.js 8 it logs

1
2
A
B
3

However, if you consider this code:

Promise.resolve()
  .then(() => console.log(1))
  .then(() => console.log(2))
  .then(() => console.log(3));

(async function () {
  console.log(await Promise.resolve("A"));
  console.log("B");
})()

In Node.js 16 it logs

1
A
B
2
3

while in Node.js 8 it logs

1
2
3
A
B

The Node.js 8 behavior is not a Node.js 8 bug: Node.js 8 was correct, but then the spec was changed to produce the Node.js 16 output. A fix for this bug (which is similar to the one fixed in this PR) would require compiling async functions to generators in many browser that otherwise supports async functions:

• Node.js 10 instead of Node.js 7.6
• Chrome 68 instead of Chrome 55
• Firefox 67 instead of Firefox 52

This breaking change in the ECMAScript specification affects a very small number of users (the majority of the web doesn't rely on the exact task queue ordering), but it can be easily worked around forcing Babel to compile async functions even when they are otherwise supported.

[nicolo-ribaudo]

The CI failure is not related to this PR.

[danieltroger]

No, this PR will be enabled by default

Amazing, thanks a lot!

About the spec change: IMO the transpiled code should produce the same output in a modern browser as the untranspiled code in a modern browser, so transpiling it for Node 16 is the right thing to do I'd say.

[danieltroger]

@nicolo-ribaudo Sorry for being impatient, what is blocking this from being merged? I'd love to start using for await in my code.

[nicolo-ribaudo]

Ping for reviews!

[danieltroger]

YEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEES. Thank you @hzoo ❤️