-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pipeline operator #6335
Pipeline operator #6335
Conversation
b060d8e
to
dbcd7f0
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5099/ EDIT by @hzoo: Better link |
Open questions:
|
There's a problem I discovered with this approach. I'm currently working on another. Should I make a PR to your branch, @jridgewell ? |
PR to my branch is fine. I'm working on fixing the "simple" call expression case (left needs to be evaluated before right) |
Ah, I've got that working. In case it saves time I'll throw up a quick gist. |
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. |
Why can't it be optimized out? var _x;
_x = 10, setInterval(() => _x++, 1000); |
|
||
const optimizeArrow = | ||
t.isArrowFunctionExpression(right) && | ||
right.params.length === 1 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be optimized also if the function has 0 parameters?
It means that |
@jridgewell You're right! Good correction :) |
Blocked by #6337. |
c1cf5a4
to
e58409b
Compare
Unblocked, both questions are resolved:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} else if (params.length > 1) { | ||
optimizeArrow = false; | ||
} | ||
} else if (t.isIdentifier(right, { name: "eval" })) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need.
var result2 = [4, 9].map(x => { | ||
var _ref3, _x; | ||
|
||
return _ref3 = (_x = x, inc(_x)), double(_ref3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, any getter that occurs during the inc
evaluation (not the actual call, but the reference) could cause mutations to x
.
|
||
var map = (fn) => (array) => array.map(fn); | ||
|
||
var result = [10,20] |> map(x => x * 20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the proposed spec, it has to be after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in that respect it works similarly to &&
and ||
as opposed to a regular math operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, this definitely isn't a compose.
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. |
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 |
@willin Can you open a new issue please? Otherwise it's easy to forget about this bug |
@willin Which version are you using? It works with 7.0.0-beta.32: https://runkit.com/embed/y78p2hwg3lzj |
.babelrc: {
"parserOpts": {
"parser": "babylon",
"plugins": [
"pipelineOperator"
]
},
"presets": [
["env", {
"targets": {
"node": "current"
}
}]
],
"plugins": [
"transform-pipeline-operator"
]
} pack:
|
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 |
https://github.com/tc39/proposal-pipeline-operator
Original code from #3159, thanks @gilbert.