Transform for the smart pipeline operator proposal #9179

thiagoarrais · 2018-12-13T17:01:14Z

Q	A
Fixed Issues?	Related to babel/proposals#29
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

This implements a first draft for the smart pipeline proposal. It is based on IIFE and does not support async/await inside the pipelines yet. That will need to be added by a subsequent PR.

Relates to https://github.com/babel/babel/projects/9#card-15461643

babel-bot · 2018-12-13T17:05:32Z

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

mAAdhaTTah · 2018-12-13T19:08:40Z

Couple quick comments:

I think we need to be passing the proposal option to the parser.
It'll be easier to maintain the various proposal if there are clear "branches" for each proposal, and then extracting shared functionality to a module used by all of the branches.
I could be wrong, but I'm not sure tests in a subfolder run. They didn't for the parser, as your test setup is pretty similar to what I initially did for the parser work. My first thought looking at the code is whether the minimal still works. Which it might! Just worth double checking.

thiagoarrais · 2018-12-13T19:39:47Z

I think we need to be passing the proposal option to the parser.

I think this is done by line 21 in babel-plugin-syntax-pipeline-operator/src/index.js but I'm not sure this is what you're talking about.

It'll be easier to maintain the various proposal if there are clear "branches" for each proposal,

Good idea. I guess a first step on this direction would be having separate visitors and switching on the parseOpts proposal.

I could be wrong, but I'm not sure tests in a subfolder run.

Yeah, they do without a problem. Maybe parser tests are different somehow?

mAAdhaTTah · 2018-12-13T21:14:09Z

Yes it is, my b.
That sounds good.
Could be.

Thanks!

nicolo-ribaudo · 2018-12-13T23:19:53Z

It'll be easier to maintain the various proposal if there are clear "branches" for each proposal,

Yeah, for example I put the two different decorators transformers in two different files (https://github.com/nicolo-ribaudo/babel/blob/33ac9a246c4b4e45f8f1b63122f4ba502f3cf32e/packages/babel-plugin-proposal-decorators/src/index.js)

thiagoarrais · 2018-12-14T00:33:10Z

Nice. I'll definitely steal that idea.

js-choi · 2018-12-14T16:49:27Z

Just confirming: This version of the transformer uses IIFEs, so it doesn’t yet support await and yield, right? (See also a similar problem with the do-expressions transformer filed in #3780.)

thiagoarrais · 2018-12-14T16:54:35Z

Yeah . You're right, @js-choi

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

js-choi · 2018-12-14T17:29:50Z

Yes, great: thanks for your hard work, @thiagoarrais.

@thiagoarrais and I have already talked about this, but to explain for other people’s benefit: Because this current transform uses IIFEs, it won’t work with operations that are dependent on the current function: namely await and yield.

In other words, even though this:

async function () { return [g(), x |> f |> # + 1]; }

…is the same as this:

async function () { return [g(), ($ => $ + 1)(f(x))]; }

…adding an await or yield messes it up:

async function () { return [g(), x |> f |> await # |> # + 1]; }

…because that isn’t the same as this, which is an error:

async function () { return [g(), ($ => $ + 1)(($ => await $)(f(x)))]; }

So this IIFE-using transform is a preliminary approach. Eventually, we’ll have to write a function that finds expressions within statements and then recursively pulls them out into constant declarations. Such a function would convert code like this:

async function () { return [g(), x |> f |> await # |> # + 1]; }

…to code like this:

async function () {
  const $1 = g();
  const $2 = x;
  const $3 = f($2);
  const $4 = await $3;
  const $5 = $4 + 1;
  return [$1, $5];
}

I suspect that, in fact, the current F#-plus-await pipeline proposal will need something like this too for its own approach to await.

In fact, the do-expression plugin, which currently also uses IIFEs, could also use such functionality, as per #3780.

But even though this version of the transform might be preliminary, it’s still a great start! Thank you, @thiagoarrais.

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

nicolo-ribaudo · 2018-12-16T23:25:45Z

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

+
+const minimalVisitor = {
+  BinaryExpression: {
+    exit(path) {


Why do we need to run this on exit?

Some of the calls in this function need Expression nodes and the smart proposal child nodes aren't expressions, but can be converted by the smart proposal visitor. Running on exit guarantees that the child nodes are already converted.

This feels a bit like the logic is getting mixed between them. This might be a bit pendantic, but I'd extract the shared logic to a separate module and run them at different times.

We could move this function to its own module and bind it to entry (or the default BinaryExpression) on minimal and to exit on smart. But I'm not really sure about that. It feels like splitting hairs.

Plus: running on exit works great with the flattening PR that I've got in the works.

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

nicolo-ribaudo · 2018-12-17T21:57:31Z

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

-        );
-      },
-    },
+    visitor: visitorsPerProposal[options.proposal],


We should add validation for the proposal optoin.

I think the syntax plugin already does this.

Do we need to add it here too?

In the decorators plugin I put the check both in the syntax and in the transform plugin, so that the error message uses the correct plugin name. It is a nice-to-have but it isn't really needed.

Is it possible for that extra check by the transform plugin to be observable by a test? All the fixtures in all the transform plugins that I’ve read seem to integration-test both parsing and transforming.

Yes, you can test the error message by adding "error": "Expected error message" in the options.json file of the fixture.

Well, even then, adding such a test fixture still wouldn’t distinguish between the parser throwing the error versus the transform plugin throwing the error, right?

Yeah that's right. But usually parser error messages are different from plugin error messages. That is the important part; not where the error actually comes from.

I find the syntax plugin message (below) is clear enough in this case.

The pipeline operator plugin requires a 'proposal' option. 'proposal' must be one of: minimal, smart. More details: https://babeljs.io/docs/en/next/babel-plugin-proposal-pipeline-operator

Maybe it should be moved to the transform plugin. What about holding it off for a later PR?

Oh I didn't noticed that the link alreay points to the transform plugin. It is ok as-is 👍

nicolo-ribaudo

Thank you! Don't know why I initially thought that this PR would have been much bigger; it looks good to me 🙂

thiagoarrais · 2018-12-18T18:54:08Z

Nice! I'm done here for now. Please let me know if you need anything else.

mAAdhaTTah · 2018-12-18T21:58:25Z

Damn, I need to hurry up!

js-choi · 2018-12-19T01:37:10Z

@thiagoarrais: Great work, again. Let me know on whether you plan to tackle #9178 (yield parsing) or the IIFEs-to-flattened-expressions rewrite. For what it’s worth, I expect to submit a pull request for adding ?-placeholder support to babel-parser within two days, time permitting, and that will rename a lot of babel-parser’s pipeline test fixtures.

@mAAdhaTTah: No need to rush! We still don’t have await/yield support yet due to the use of IIFEs, after all. But feel free to keep working independently as you can; let me know if you need any help.

thiagoarrais · 2018-12-24T21:08:58Z

Since, #9216 landed, I've rebased to the latest master and added a check for yield.

thiagoarrais · 2018-12-29T20:48:10Z

Bumping this. Let me know if you need anything else, @nicolo-ribaudo.

nicolo-ribaudo · 2018-12-29T20:50:54Z

It looks good to me. I didn't merge it because we have a policy of two ✔️ from core team members, but reviews by @mAAdhaTTah and @js-choi would probably be equally valuable on this topic.

mAAdhaTTah

Nice job! Happy to see this moving forward. Let some questions / suggestions.

mAAdhaTTah · 2018-12-31T00:49:05Z

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

+  const visitorsPerProposal = {
+    minimal: minimalVisitor,
+    smart: smartVisitor,
+  };


Hoist to module root?

Good idea. Wil do it.

mAAdhaTTah · 2018-12-31T01:00:17Z

...l-plugin-proposal-pipeline-operator/test/fixtures/smart/topic-inside-arrow-function/input.js

@@ -0,0 +1,5 @@
+const result = -2.2   // -2.2
+  |> Math.floor       // -3
+  |> (() => Math.pow(#, 5))   // () => -243


@js-choi Question on the spec behavior: if we didn't use a topic token here, this arrow would get executed?

-2.2 |> (x => Math.pow(x, 5))

This is also valid in Smart. Are we checking whether the token appears in the arrow body?

mAAdhaTTah · 2018-12-31T01:01:40Z

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

+
+const minimalVisitor = {
+  BinaryExpression: {
+    exit(path) {


This feels a bit like the logic is getting mixed between them. This might be a bit pendantic, but I'd extract the shared logic to a separate module and run them at different times.

xtuc

LGTM (I don't know the spec well tho), thanks for the PR!

No support for await yet

@nicolo-ribaudo

Thanks, @nicolo-ribaudo!

hzoo · 2019-01-21T23:29:46Z

Released in 7.3! Thanks everyone https://twitter.com/NicoloRibaudo/status/1087473662696017922?s=20

thiagoarrais · 2019-01-22T00:50:12Z

Thank you everyone! I'll start work on flattening expressions (async/await) next.

nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Pipeline Operator labels Dec 13, 2018

nicolo-ribaudo added this to In progress in Pipeline Operator via automation Dec 13, 2018

nicolo-ribaudo self-requested a review December 13, 2018 21:45

thiagoarrais commented Dec 14, 2018

View reviewed changes

packages/babel-plugin-proposal-pipeline-operator/src/index.js Outdated Show resolved Hide resolved

nicolo-ribaudo reviewed Dec 16, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

nicolo-ribaudo reviewed Dec 17, 2018

View reviewed changes

nicolo-ribaudo approved these changes Dec 18, 2018

View reviewed changes

thiagoarrais force-pushed the smart-pipeline-transform branch from 04d662f to 7cef23b Compare December 18, 2018 18:52

js-choi mentioned this pull request Dec 19, 2018

[smart pipes] Add support for question-mark tokens #9209

Closed

thiagoarrais force-pushed the smart-pipeline-transform branch from 7cef23b to 1c2221f Compare December 24, 2018 21:04

mAAdhaTTah reviewed Dec 31, 2018

View reviewed changes

loganfsmyth self-requested a review January 14, 2019 17:17

nicolo-ribaudo mentioned this pull request Jan 14, 2019

Babel 7.3.0 #9334

Closed

3 tasks

xtuc approved these changes Jan 17, 2019

View reviewed changes

thiagoarrais added 6 commits January 21, 2019 13:29

Smart pipelines transform - first draft

bd48e99

No support for await yet

Split pipeline proposals per file

0ac0131

Avoid extra traversal

48db11f

Thanks, @nicolo-ribaudo!

Adds test for smart topic as method callee

79f3c6e

Yield is not supported yet

dc31989

Hoist proposals to module root

93dcf97

thiagoarrais force-pushed the smart-pipeline-transform branch from 1c2221f to 93dcf97 Compare January 21, 2019 16:33

nicolo-ribaudo merged commit e9331aa into babel:master Jan 21, 2019

Pipeline Operator automation moved this from In progress to Done Jan 21, 2019

thiagoarrais deleted the smart-pipeline-transform branch January 22, 2019 16:13

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

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

Transform for the smart pipeline operator proposal #9179

Transform for the smart pipeline operator proposal #9179

Conversation

thiagoarrais commented Dec 13, 2018 • edited

babel-bot commented Dec 13, 2018 • edited

mAAdhaTTah commented Dec 13, 2018

thiagoarrais commented Dec 13, 2018

mAAdhaTTah commented Dec 13, 2018

nicolo-ribaudo commented Dec 13, 2018

thiagoarrais commented Dec 14, 2018

js-choi commented Dec 14, 2018 • edited

thiagoarrais commented Dec 14, 2018

js-choi commented Dec 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoarrais Jan 18, 2019 • edited

Choose a reason for hiding this comment

This comment has been minimized.

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

js-choi Dec 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js-choi Dec 18, 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

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

thiagoarrais commented Dec 18, 2018

mAAdhaTTah commented Dec 18, 2018

js-choi commented Dec 19, 2018 • edited

thiagoarrais commented Dec 24, 2018

thiagoarrais commented Dec 29, 2018

nicolo-ribaudo commented Dec 29, 2018

mAAdhaTTah left a 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

xtuc left a comment

Choose a reason for hiding this comment

hzoo commented Jan 21, 2019

thiagoarrais commented Jan 22, 2019

thiagoarrais commented Dec 13, 2018 •

edited

babel-bot commented Dec 13, 2018 •

edited

js-choi commented Dec 14, 2018 •

edited

js-choi commented Dec 14, 2018 •

edited

thiagoarrais Jan 18, 2019 •

edited

js-choi Dec 18, 2018 •

edited

js-choi Dec 18, 2018 •

edited

js-choi commented Dec 19, 2018 •

edited