Add basic support for await with the pipeline operator #7154

ksmithbaylor · 2018-01-04T15:41:43Z

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

This correctly transforms AwaitExpressions which appear in pipelines, as shown here. It does not support standalone await keywords, as was the case in earlier versions of the proposal.

babel-bot · 2018-01-04T15:58:10Z

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

babel-bot · 2018-01-04T15:58:10Z

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

nicolo-ribaudo · 2018-01-07T10:02:59Z

Can you transform async functions in the tests? Otherwise they will fail on node <= 6

ksmithbaylor · 2018-01-08T01:54:30Z

I updated the PR. Since I'd like to test both "pure" async functions and also those transformed to generators by transform-async-to-generator, I duplicated the test directory. I marked the original one as requiring Node 7.6.0 or above, and enabled the transform-async-to-generator plugin on the other one. This required some code changes, since this plugin should now handle yield expressions in addition to async.

It isn't clear to me that this is allowed according to the spec draft, however, and I expect that I could be doing the transformation more elegantly. This is my first time looking at the Babel source, and I'm still learning my way around. I'd appreciate feedback!

nicolo-ribaudo · 2018-01-10T08:39:59Z

packages/babel-plugin-proposal-pipeline-operator/src/index.js

@@ -15,6 +15,8 @@ export default function() {

        let optimizeArrow =
          t.isArrowFunctionExpression(right) && t.isExpression(right.body);
+        const isAwait = t.isAwaitExpression(right);
+        const isYield = t.isYieldExpression(right);


foo |> yield bar shouldn't be transfrmed; it should become something like (_temp = yield bar, _temp(foo)).

If there isn't a way of transforming |> await foo before transforming the async function, you can workaround the problem by adding node.__wasAwait to yield expressions generated by the async functions plugin and then do

const isAwait = t.isAwaitExpression(right) || right.__wasAwait;

ksmithbaylor · 2018-01-18T12:42:19Z

Alright, should be better now. I couldn't figure out a way to ensure that the pipeline operator plugin ran before the async-to-generator plugin, since it operates on a BinaryExpression which doesn't have a consistent parent to traverse down from. So I took your suggestion of adding a flag to await expressions that have been transformed to yield. I also added a test case that makes sure yield expressions aren't transformed.

Andarist · 2018-01-18T12:52:32Z

Are special flags a common practice? I have not yet seen many of those and it seems a clunky solution (although might be necessary ofc). We really should add plugin ordering to solve such issues

ksmithbaylor · 2018-01-18T13:31:02Z

@Andarist no, I don't really see any other examples of special flags either. I'd love to find a better way to do the check, since it does seem clunky like you said. This is my first time looking at the codebase, so I have lots of learning to do :)

As I understand it, there is some discussion around what it would look like to allow node visitors to be ordered more predictably, but a solution hasn't been decided on yet. If you know of an easier way to either force this plugin to run first or make sure yield expressions are only transformed if they were previously await expressions, I'd be open to suggestions.

Andarist · 2018-01-18T15:28:49Z

Hm, Im also still learning and I dont know about all intricacies of babel transformations.

I thought that babel's visitor is "top down" and I would suspect that this BinaryExpression would be visited before AwaitExpression (as former is the parent). But it seems that this is not true here, right?

ksmithbaylor · 2018-01-18T17:23:03Z

That's what I thought, but when I combined the pipeline plugin with the async-to-generator one, all the AwaitExpressions had already been transformed by the time the pipeline's BinaryExpression visitor was called.

hzoo · 2018-01-18T17:27:03Z

packages/babel-helper-remap-async-to-generator/src/index.js

@@ -13,6 +13,7 @@ const awaitVisitor = {

  AwaitExpression(path, { wrapAwait }) {
    const argument = path.get("argument");
+    argument.node.__wasAwait = true;


I think we wanted to stay away from using a hidden properties? Or we need a more formalized way to handle this kind of thing (like knowing if it was transformed or original source, and what it used to be)

I agree. Do you have any suggestions that could help either act as an indicator or force these two plugins to run in order that wouldn't require blocking until those issues are solved at a higher level?

Unfortunately no, it's related to #5854

Inside the same plugin you can use a WeakMap/WeakSet.

Plugin ordering definitely makes this harder. Could we make this throw an error if the async function tries to transform an await that is inside of a pipeline expression, and then have the pipeline transform have a visitor for async functions that does an explicit sub-traversal for pipeline expressions? Then we could just make sure that the pipeline plugin runs before the async function transform and I think we'd be okay?

That sounds good to me, we don't need to order the plugins if they are both idenpendent. Each will transform what it needs.

Andarist · 2018-01-18T18:29:51Z

...roposal-pipeline-operator/test/fixtures/pipeline-operator/async-with-transform-async/exec.js

+  return 42 |> (yield 10);
+}
+
+var fooGen;


any reason to have this outside of the promise?

Oops, nope. That was left-over from moving something around.

Andarist · 2018-01-18T19:58:04Z

...posal-pipeline-operator/test/fixtures/pipeline-operator/async-with-transform-async/actual.js

@@ -0,0 +1,10 @@
+var incPromise = (x) => Promise.resolve(x + 1);


i'd prefer to have the same code in actual / exec if they are in the same directory, exec differs slightly right now and it makes it hard to compare those files (assert ofc should stay just in exec)

I made them match now. I think I may have saved the file with Prettier on at some point.

Oh, I didnt mean that they do not match in sense of formatting - i think there was some differenc in how incFuncPromise was used

Oh, got it. I also changed the usage of that variable down below. Either way, they're the same now.

Andarist · 2018-01-18T19:58:58Z

...sal-pipeline-operator/test/fixtures/pipeline-operator/async-with-transform-async/expected.js

+function* foo() {
+  var _3;
+
+  return _3 = 42, (yield incFuncPromise)(_3);


was this intentional? looks weird, but i guess it could test something, but then exec should reflect that test

Andarist · 2018-01-18T20:02:33Z

@ksmithbaylor regarding why AwaitExpression gets visited first - I think its because actually its not visited from a "top level" traversal - there is a traverse call from some other visitor and that's why whole subtree is visited first there (i guess traverse is called in async/generator function, which is a parent of that BinaryExpression). I have not tested this, but I believe that's the case.

nicolo-ribaudo · 2018-01-18T20:49:11Z

@Andarist You are right: @babel/helper-remap-async-to-generator transforms await expressions by spawning a new traversal:

babel/packages/babel-helper-remap-async-to-generator/src/index.js

Lines 66 to 69 in 47ce7e7

    
           path.traverse(awaitVisitor, { 
        
             file, 
        
             wrapAwait: helpers.wrapAwait, 
        
           });

I think that if we transform async functions on exit this problem would go away, but probably it would make it be traversed one more time.

Andarist · 2018-01-18T21:22:38Z

I think that if we transform async functions on exit this problem would go away, but probably it would make it be traversed one more time.

Why would it be traversed twice?

EDIT: oh yeah, content of the async function would get traversed twice, because after remapping it would get requeued, right?

nicolo-ribaudo · 2018-01-18T21:31:02Z

Actually it is already traversed twice (the main traversal and the one spawned by the helper); it would be traversed three times.

oh yeah, content of the async function would get traversed twice, because after remapping it would get requeued, right?

Yes 👍

nicolo-ribaudo · 2018-01-18T21:32:15Z

packages/babel-plugin-proposal-pipeline-operator/src/index.js

@@ -15,6 +15,8 @@ export default function() {

        let optimizeArrow =
          t.isArrowFunctionExpression(right) && t.isExpression(right.body);
+        const wasAwait = right.argument && right.argument.__wasAwait;


Maybe

const wasAwait = t.isYieldExpression(right) && right.argument && right.argument.__wasAwait;

?

Probably a good idea.

nicolo-ribaudo · 2018-01-18T21:32:59Z

...roposal-pipeline-operator/test/fixtures/pipeline-operator/async-with-transform-async/exec.js

@@ -0,0 +1,21 @@
+var incPromise = x => Promise.resolve(x + 1);
+var incFuncPromise = Promise.resolve(x => x + 10);


This is not used.

ksmithbaylor · 2018-02-03T15:54:30Z

Does anyone have ideas on how to improve this without relying on special flags?

mAAdhaTTah · 2018-03-07T22:34:27Z

Wondering if it would be acceptable to land this w/o supporting transformed await and revist that issue in the future?

Also, I thought this had labels for Spec: Pipeline previously, but I don't see them, could they be added?

mAAdhaTTah · 2018-03-18T14:07:51Z

So this PR may not actually be needed. We're going to ban await in the basic, "minimal proposal" spec here: tc39/proposal-pipeline-operator#108 and solve await in each of the proposals we're working on currently working on. This is not going to be the final semantics of pipeline in any of the proposals, given the ambiguity between whether x |> await f becomes await f(x) or (await f)(x), so I think we can just close this.

ksmithbaylor · 2018-03-19T20:01:00Z

@mAAdhaTTah agreed!

ksmithbaylor force-pushed the master branch from 173784a to 3161119 Compare January 4, 2018 15:44

Add support for await with the pipeline operator

5d539fd

ksmithbaylor force-pushed the master branch from 3161119 to 5d539fd Compare January 4, 2018 15:47

Handle yield expressions and transform-async in pipeline operator

016784a

nicolo-ribaudo requested changes Jan 10, 2018

View reviewed changes

Do not transform non-await yield expressions for pipeline operator

9c78ae8

Remove argument destructuring to fix tests in Node 4

834ad9c

hzoo reviewed Jan 18, 2018

View reviewed changes

Andarist reviewed Jan 18, 2018

View reviewed changes

Move variable declaration inline

cc95f94

Andarist reviewed Jan 18, 2018

View reviewed changes

nicolo-ribaudo reviewed Jan 18, 2018

View reviewed changes

ksmithbaylor added 2 commits January 19, 2018 05:18

Make exec and actual files match

176c038

Add check for yield expression

3c5b19a

nicolo-ribaudo mentioned this pull request Mar 4, 2018

Pipeline operator doesnt await properly #7480

Closed

This was referenced Mar 8, 2018

Discussion: Proposal Mixes tc39/proposal-pipeline-operator#89

Closed

Enable parsing of |> await #7458

Closed

mAAdhaTTah mentioned this pull request Mar 11, 2018

Expose a prefix that plugins can use to annotate labelled statements and directives so they will be stripped out. #7544

Closed

nicolo-ribaudo added the Spec: Pipeline Operator label Mar 11, 2018

ksmithbaylor closed this Mar 19, 2018

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

		@@ -0,0 +1,10 @@
		var incPromise = (x) => Promise.resolve(x + 1);

		@@ -0,0 +1,21 @@
		var incPromise = x => Promise.resolve(x + 1);
		var incFuncPromise = Promise.resolve(x => x + 10);

Add basic support for await with the pipeline operator #7154

Add basic support for await with the pipeline operator #7154

Conversation

ksmithbaylor commented Jan 4, 2018

babel-bot commented Jan 4, 2018 • edited

babel-bot commented Jan 4, 2018

nicolo-ribaudo commented Jan 7, 2018

ksmithbaylor commented Jan 8, 2018

Choose a reason for hiding this comment

ksmithbaylor commented Jan 18, 2018

Andarist commented Jan 18, 2018

ksmithbaylor commented Jan 18, 2018

Andarist commented Jan 18, 2018

ksmithbaylor commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jan 18, 2018

nicolo-ribaudo commented Jan 18, 2018

Andarist commented Jan 18, 2018 • edited

nicolo-ribaudo commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksmithbaylor commented Feb 3, 2018

mAAdhaTTah commented Mar 7, 2018 • edited

mAAdhaTTah commented Mar 18, 2018

ksmithbaylor commented Mar 19, 2018

babel-bot commented Jan 4, 2018 •

edited

xtuc Mar 8, 2018 •

edited

Andarist commented Jan 18, 2018 •

edited

mAAdhaTTah commented Mar 7, 2018 •

edited