-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Optimize transform-async-to-generator
output
#14122
Conversation
- remove wrapper if the function length is zero. - remove wrapper if the `assumptions.ignoreFunctionLength` is `true`.
const params = node.params.reduce( | ||
(acc, param) => { | ||
acc.done = acc.done || isAssignmentPattern(param) || isRestElement(param); | ||
|
||
if (!acc.done) { | ||
acc.params.push(path.scope.generateUidIdentifier("x")); | ||
} | ||
|
||
return acc; | ||
}, | ||
{ | ||
params: [], | ||
done: false, | ||
}, | ||
).params; |
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 know that you didn't write this code but you just moved it from a few lines below, however it's an example of "overusing reduce" and we can probably simplify it since we are touching these lines anyway:
const params = node.params.reduce( | |
(acc, param) => { | |
acc.done = acc.done || isAssignmentPattern(param) || isRestElement(param); | |
if (!acc.done) { | |
acc.params.push(path.scope.generateUidIdentifier("x")); | |
} | |
return acc; | |
}, | |
{ | |
params: [], | |
done: false, | |
}, | |
).params; | |
const params = []; | |
for (const param of node.params) { | |
if (isAssignmentPattern(param) || isRestElement(param)) break; | |
params.push(path.scope.generateUidIdentifier("x")); | |
} |
})(); | ||
|
||
foo(async (...x) => {}); | ||
foo(async (...[...y]) => {}); |
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.
All these functions would have length: 0
anyway: to test the behavior of ignoreFunctionLength
, we shuold use functions with .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.
Thanks.
I was writing on the first case and accidentally mixed it up.
I will update this test case.
foo(async function ({}) {}); | ||
|
||
export default async ([...x]) => {}; | ||
|
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.
There is one more case that we should test:
export default async function (x) {}
The function should still be wrapped, because it is a function declaration and it needs to be hoisted.
Why?
Otherwise this code doesn't work, when a.js
is the entrypoint:
// a.js
import x from "./b.js";
x.then(console.log);
export default async function () { return 1 }
// b.js
import f from "./a.js";
export default f();
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.
Done. test added.
if (!retFunction || retFunction.id || node.params.length) { | ||
if ( | ||
!retFunction || | ||
retFunction.id || |
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.
Probably we can do it by adding || isFunctionDeclaration(retFunction)
here.
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.
We don't need to change it.
default export use a different template builder.
The wrapper is in another function.
You can check the test case.
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.
Oh awesome, thanks for the test!
assumptions.ignoreFunctionLength
istrue
.