Pipeline operator by jridgewell · Pull Request #6335 · babel/babel

jridgewell · 2017-09-29T04:13:24Z

https://github.com/tc39/proposal-pipeline-operator

Original code from #3159, thanks @gilbert.

babel-bot · 2017-09-29T04:23:19Z

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

EDIT by @hzoo: Better link

jridgewell · 2017-09-29T04:25:53Z

@gilbert @littledan:

Open questions:

All calls to eval are indirect eval.
- I don't actually understand direct and indirect eval, so no idea if the transform follows this.
The pipeline operator is eligible for proper tail calls, when appropriate.
- Are sequence expressions TCO? If not, we'll need to move the return inside.

gilbert · 2017-09-29T04:32:26Z

There's a problem I discovered with this approach. I'm currently working on another. Should I make a PR to your branch, @jridgewell ?

jridgewell · 2017-09-29T04:36:20Z

PR to my branch is fine. I'm working on fixing the "simple" call expression case (left needs to be evaluated before right)

gilbert · 2017-09-29T04:48:37Z

Ah, I've got that working. In case it saves time I'll throw up a quick gist.

gilbert · 2017-09-29T04:50:08Z

https://gist.github.com/gilbert/b8d7a8e8506c7443da479ffaff3730d1

gilbert · 2017-09-29T04:58:22Z

The problem I discovered was arrow functions can only be optimized out if the parameter has no closure attached to it. For example, this case cannot be optimized out:

10 |> x => setInterval(() => x++, 1000)

I'm not sure if you can analyze closures in babel, but if you can then it's possible to ignore these arrows while optimizing the others.

jridgewell · 2017-09-29T05:15:34Z

Why can't it be optimized out?

var _x;

_x = 10, setInterval(() => _x++, 1000);

nicolo-ribaudo · 2017-09-29T05:42:06Z

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

+
+        const optimizeArrow =
+          t.isArrowFunctionExpression(right) &&
+          right.params.length === 1 &&


Can this be optimized also if the function has 0 parameters?

nicolo-ribaudo · 2017-09-29T05:46:13Z

All calls to eval are indirect eval.

It means that x |> eval is not eval(x), but anything else that would be equivalent to it (e.g. (0, eval)(x))

gilbert · 2017-09-29T17:34:09Z

@jridgewell You're right! Good correction :)

jridgewell · 2017-09-29T18:03:32Z

Blocked by #6337.

jridgewell · 2017-09-29T23:37:07Z

Unblocked, both questions are resolved:

All calls to eval are indirect eval
- Evals now use (0, eval)()
The pipeline operator is eligible for proper tail calls, when appropriate.
- return 1, call() is TCO, so we're already good.

littledan

I'm not 100% sure on the syntax (will review), but the transform LGTM. I appreciate the attention to detail and asking questions on the thread that led here.

Jessidhia · 2017-10-02T01:51:34Z

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

+          } else if (params.length > 1) {
+            optimizeArrow = false;
+          }
+        } else if (t.isIdentifier(right, { name: "eval" })) {


I thought it might be worth it to do a path.scope check to see if this is the global eval or something else with the same name, but it turns out an eval call is not considered indirect if the identifier name is eval, regardless of whether it is the global eval or a local.

This should be TypeError (or undefined if sloppy) in indirect eval, but works:

(eval => { const x = 'test'; eval('console.log(x)') })(eval) //=> test

Renaming the function argument to anything else makes it correctly TypeError.

Jessidhia · 2017-10-02T02:00:41Z

...ugin-transform-pipeline-operator/test/fixtures/pipeline-operator/arrow-functions/expected.js

@@ -0,0 +1,15 @@
+var _ref, _ref2, _sum;
+
+var result = (_ref = [5, 10], (_ref2 = _ref.map(x => x * 2), (_sum = _ref2.reduce((a, b) => a + b), _sum + 1)));


I'm definitely overthinking this, but I wonder if it would be good to "clean up" the refs after the expression is evaluated.

Say this is an outer function that calls a long lived function (e.g. an event loop); these live references will forever prevent GC of the temporary arrays.

This idea, even if feasible, is probably out of the scope of this PR, though.

Any idea of how we would clean up? Do we do anything like this in any other transform, since I don't know of it?

I don't think there's a need.

Jessidhia · 2017-10-02T02:02:47Z

...ugin-transform-pipeline-operator/test/fixtures/pipeline-operator/arrow-functions/expected.js

+var result2 = [4, 9].map(x => {
+  var _ref3, _x;
+
+  return _ref3 = (_x = x, inc(_x)), double(_ref3);


This _x feels wasteful but I guess it works to guard against mutations on the right side (e.g. ++ operator)

EDIT: this is exactly why it is done according to earlier discussion in the PR 😅

Yah, any getter that occurs during the inc evaluation (not the actual call, but the reference) could cause mutations to x.

Jessidhia · 2017-10-02T02:05:23Z

.../babel-plugin-transform-pipeline-operator/test/fixtures/pipeline-operator/currying/actual.js

+
+var map = (fn) => (array) => array.map(fn);
+
+var result = [10,20] |> map(x => x * 20);


(clarification) is the map(x => x * 20) call to create the actually applied function supposed to not happen until after the LHS of |> is evaluated?

Per the proposed spec, it has to be after.

So in that respect it works similarly to && and || as opposed to a regular math operator.

Yah, this definitely isn't a compose.

willin · 2017-11-16T09:16:51Z

bug:

 ctx.data = await adminList({ page, size })
    |> (([[{ count }], result]) => ({
      page: parseInt(page, 10),
      pages: Math.ceil(count / size),
      size: parseInt(size, 10),
      total: count,
      list: result
    }));

await func result:

[ [ RowDataPacket { count: 2 } ],
  [ RowDataPacket {
      uid: 1,
      createdat: 0,
      updatedat: 1510195951,
      user: [RowDataPacket] },
    RowDataPacket {
      uid: 12021,
      createdat: 1510733990,
      updatedat: 1510733990,
      user: [RowDataPacket] } ] ]

run result:

ReferenceError: count is not defined

export default async (ctx) => {
  const { page = 1, size = 50 } = ctx.query;
  ctx.data = await adminList({ page, size })
    |> (result => ({
      page: parseInt(page, 10),
      pages: Math.ceil(result[0][0].count / size),
      size: parseInt(size, 10),
      total: result[0][0].count,
      list: result[1]
    }));
};
// can work well ... but it's too silly.

willin · 2017-11-16T09:21:11Z

test case:

const data = [
  [{ count: 2 }],
  [
    { uid: 1 },
    { uid: 2 }
  ]
];

const test = data |> (([[{ count }], result]) => ({
  total: count,
  list: result
}));

console.log(test);
// error

const data = [
  [{ count: 2 }],
  [
    { uid: 1 },
    { uid: 2 }
  ]
];

data |> (([[{ count }], result]) => {
  return {
    total: count,
    list: result
  }
}) |> console.log
// ok

nicolo-ribaudo · 2017-11-16T14:56:59Z

@willin Can you open a new issue please? Otherwise it's easy to forget about this bug

nicolo-ribaudo · 2017-11-16T16:28:00Z

@willin Which version are you using? It works with 7.0.0-beta.32: https://runkit.com/embed/y78p2hwg3lzj

willin · 2017-11-17T00:46:25Z

.babelrc:

{
  "parserOpts": {
    "parser": "babylon",
    "plugins": [
      "pipelineOperator"
    ]
  },
  "presets": [
    ["env", {
      "targets": {
        "node": "current"
      }
    }]
  ],
  "plugins": [
    "transform-pipeline-operator"
  ]
}

pack:

"dependencies": {
    "babel-polyfill": "^6.26.0",
    "babel-register": "^6.26.0",
    "babel-runtime": "^6.26.0"
  },
  "devDependencies": {
    "babel-cli": "^6.26.0",
    "babel-core": "^6.26.0",
    "babel-loader": "^7.1.2",
    "babel-minify-webpack-plugin": "^0.2.0",
    "babel-plugin-transform-pipeline-operator": "^7.0.0-beta.3",
    "babel-preset-env": "^1.6.0",
    "babylon": "^7.0.0-beta.31"
  }

hzoo · 2017-11-17T01:19:32Z

This isn't a bug with the transform so I'd ask you don't comment on the PR next time?

You are using 1 plugin with 7.x while everything else is 6.x

jridgewell force-pushed the pipeline branch from b060d8e to dbcd7f0 Compare September 29, 2017 04:14

nicolo-ribaudo reviewed Sep 29, 2017

View reviewed changes

hzoo added area: tc39 proposal PR: New Feature 🚀 A type of pull request used for our changelog categories labels Sep 29, 2017

hzoo mentioned this pull request Sep 29, 2017

Pipeline Operator (Stage 1) babel/proposals#29

Open

2 tasks

gilbert and others added 4 commits September 29, 2017 19:01

Pipeline operator

81496ab

Ensure left is evaluated before right

60335ce

Optimize 0 param arrow

4b44011

Indirect eval

e58409b

jridgewell force-pushed the pipeline branch from c1cf5a4 to e58409b Compare September 29, 2017 23:01

littledan approved these changes Sep 30, 2017

View reviewed changes

This was referenced Sep 30, 2017

Babel plugin tc39/proposal-pipeline-operator#32

Closed

babel-plugin-transform-function-composition tc39/proposal-pipeline-operator#42

Closed

Babel plugin tc39/proposal-pipeline-operator#18

Closed

Jessidhia approved these changes Oct 2, 2017

View reviewed changes

hzoo merged commit 4a8137c into babel:master Oct 2, 2017

jridgewell deleted the pipeline branch October 2, 2017 20:41

hzoo added the Spec: Pipeline Operator label Oct 5, 2017

hzoo removed the area: tc39 proposal label Oct 5, 2017

jridgewell restored the pipeline branch April 9, 2018 17:23

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,15 @@
		var _ref, _ref2, _sum;

		var result = (_ref = [5, 10], (_ref2 = _ref.map(x => x * 2), (_sum = _ref2.reduce((a, b) => a + b), _sum + 1)));


		var map = (fn) => (array) => array.map(fn);

		var result = [10,20] \|> map(x => x * 20);

Uh oh!

Conversation

jridgewell commented Sep 29, 2017

Uh oh!

babel-bot commented Sep 29, 2017 • edited by hzoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilbert commented Sep 29, 2017

Uh oh!

jridgewell commented Sep 29, 2017

Uh oh!

gilbert commented Sep 29, 2017

Uh oh!

gilbert commented Sep 29, 2017

Uh oh!

gilbert commented Sep 29, 2017

Uh oh!

jridgewell commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Sep 29, 2017

Uh oh!

gilbert commented Sep 29, 2017

Uh oh!

jridgewell commented Sep 29, 2017

Uh oh!

jridgewell commented Sep 29, 2017

Uh oh!

littledan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jessidhia Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jessidhia Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willin commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willin commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Nov 16, 2017

Uh oh!

nicolo-ribaudo commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willin commented Nov 17, 2017

Uh oh!

hzoo commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

babel-bot commented Sep 29, 2017 •

edited by hzoo

Loading

jridgewell commented Sep 29, 2017 •

edited

Loading

jridgewell commented Sep 29, 2017 •

edited

Loading

Jessidhia Oct 2, 2017 •

edited

Loading

Jessidhia Oct 2, 2017 •

edited

Loading

willin commented Nov 16, 2017 •

edited

Loading

willin commented Nov 16, 2017 •

edited

Loading

nicolo-ribaudo commented Nov 16, 2017 •

edited

Loading