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
Improve output when wrapping functions #15992
base: main
Are you sure you want to change the base?
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56931 |
39776f4
to
61b2cee
Compare
7da248f
to
bf5b052
Compare
} | ||
`; | ||
|
||
helpers.asyncToGenerator2 = helper("7.23.0")` |
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.
Does this have different behavior than the old asyncToGenerator
? If not, we can just replace the old one.
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.
No, I'm just not sure if this affects the compatibility of transform-runtime
and helpers
, I'll replace it.
? callExpression(cloneNode(wrapAwait), [argument.node]) | ||
? callExpression( | ||
typeof wrapAwait === "string" | ||
? path.hub.addHelper(wrapAwait) |
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 have multiple issues with path.hub
, maybe we could make wrapAwait
be a t.Expression | () => t.Expression
and pass it as wrapAwait: () => file.addHelper("...")
from the caller?
|
||
function markCallWrapped(path: NodePath<t.Function>) { | ||
(path.get("body.body.0.argument") as NodePath).setData( | ||
"babel-helper-wrap-function_wrapped_function", |
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.
Let's extract this string to a constant, or maybe even just use a weakmap.
@@ -47,6 +47,7 @@ export default declare(api => { | |||
inherits: syntaxFunctionSent, | |||
|
|||
visitor: { | |||
...wrapFunction.buildOnCallExpression("callSkipFirstGeneratorNext"), |
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 should use traverse.visitor.merge
, in case we will add visitors that cover the same nodes.
export function buildOnCallExpression(helperName: string) { | ||
return { | ||
CallExpression: { | ||
exit(path: NodePath<t.CallExpression>, state: PluginPass) { |
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.
Why do we need to do this on exit, and not when we call wrapFunction
?
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.
https://github.com/babel/babel/pull/15992/files#diff-84a64030b885f05d8fef6726ef5a9c863a2761756c846ff5ca9278a058384151L1-L11
In the past, we always created a closure for the function to save, which was necessary for use cases like babelHelpers.regeneratorRuntime().mark()
.
Now, since more and more users support generators natively, I chose not to create closures for all functions, but only save the results for use cases like regeneratorRuntime
.
The generator function transformation runs after wrapFunction
, and if we modify that plugin directly, we will have to deal with compatibility issues.
To be honest, after I finished it, I thought it was more complicated than I expected, but it's done and there's no harm in it.
name: "John Doe", | ||
testMethodFailure() { | ||
var _this = this; | ||
return new Promise( /*#__PURE__*/function () { | ||
var _ref = babelHelpers.asyncToGenerator(function* (resolve) { | ||
return new Promise(function (_x) { | ||
return babelHelpers.callAsync(function* (resolve) { | ||
console.log(_this); | ||
setTimeout(resolve, 1000); | ||
}); | ||
return function (_x) { | ||
return _ref.apply(this, arguments); | ||
}; | ||
}()); | ||
}, this, arguments); | ||
}); | ||
} | ||
}; |
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.
If we don't use OnCallExpression
then we will have to generate output like this.
let TestClass = {
name: "John Doe",
testMethodFailure() {
var _this = this;
return new Promise(function (_x) {
return (ref =
ref ||
babelHelpers.asyncToGenerator(function* (resolve) {
console.log(_this);
setTimeout(resolve, 1000);
})).apply(this, arguments);
});
}
};
@@ -34,38 +34,45 @@ helpers.wrapAsyncGenerator = helper("7.0.0-beta.0")` | |||
} | |||
`; | |||
|
|||
helpers.asyncToGenerator = helper("7.0.0-beta.0")` |
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 sure if the version should be modified.
I will add new-version-checklis
after #15959 is merged.
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.
@liuxingbaoyu Just a kind reminder that #15959 has been merged.
return new Promise(function (resolve, reject) { | ||
function step(key, arg) { | ||
try { | ||
var info = gen[key](arg); |
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.
While we're here, we can gain ~30% performance improvement by changing key
into a boolean, and changing this to happy ? gen.next(arg) : gen.throw(arg)
} | ||
function _fn() { | ||
_fn = babelHelpers.asyncToGenerator(function* () { | ||
return babelHelpers.callAsync(function* () { |
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.
Was the intention of the old code to keep this as a top-level function, so we don't need to reevaluate it constantly? I don't think it achieved that, but it would help overall performance.
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.
Initially it was a variable declaration + a function declaration, but later it became two function declarations to fix the hoisting problem.
Perhaps initially creating a new closure might be to facilitate working with bluebird
/regenerator
.
Because they're better off calling regenerator.mark
/bluebird.coroutine
only once on the function body.
(I'm not sure about this, just guessing)
The original function declaration was probably to preserve fn.length
and fn.name
.
To be honest, after finishing it I found it to be more complicated than I expected.
The main reason is that we need to cache the results of
_regeneratorRuntime().mark()
to avoid performance regression.The reason the old code cannot be removed is that we still support
bluebird-coroutines
.