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
Support transforming params of arrow functions in class fields #13941
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
let f = (x = 0) => x + 1; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
var _this = this; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is another bug fixed by the new |
||
|
||
let f = function f() { | ||
let x = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : 0; | ||
babelHelpers.newArrowCheck(this, _this); | ||
return x + 1; | ||
}.bind(this); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"plugins": ["transform-parameters"], | ||
"assumptions": { | ||
"noNewArrows": false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
class A extends B { | ||
handle = ((x = 0) => { | ||
console.log(x, this, new.target, super.y); | ||
})(() => { | ||
let y = 0; | ||
return (x = y) => x + this; | ||
})((x = 1) => {})(this); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"plugins": [ | ||
"transform-parameters" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
class A extends B { | ||
handle = (() => { | ||
var _newtarget = new.target, | ||
_superprop_getY = () => super.y, | ||
_this = this; | ||
|
||
return function () { | ||
let x = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : 0; | ||
console.log(x, _this, _newtarget, _superprop_getY()); | ||
}; | ||
})()(() => { | ||
var _this2 = this; | ||
|
||
let y = 0; | ||
return function () { | ||
let x = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : y; | ||
return x + _this2; | ||
}; | ||
})((() => function () { | ||
let x = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : 1; | ||
})())(this); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class A { | ||
handle = (x = 0) => { | ||
console.log(x); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"plugins": [ | ||
"transform-parameters" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
class A { | ||
handle = (() => function () { | ||
let x = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : 0; | ||
console.log(x); | ||
})(); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -22,6 +22,7 @@ import { | |||||
stringLiteral, | ||||||
super as _super, | ||||||
thisExpression, | ||||||
toExpression, | ||||||
unaryExpression, | ||||||
} from "@babel/types"; | ||||||
import type * as t from "@babel/types"; | ||||||
|
@@ -146,27 +147,26 @@ export function arrowFunctionToExpression( | |||||
); | ||||||
} | ||||||
|
||||||
const thisBinding = hoistFunctionEnvironment( | ||||||
const { thisBinding, fnPath: fn } = hoistFunctionEnvironment( | ||||||
this, | ||||||
noNewArrows, | ||||||
allowInsertArrow, | ||||||
); | ||||||
|
||||||
this.ensureBlock(); | ||||||
// @ts-expect-error todo(flow->ts): avoid mutating nodes | ||||||
this.node.type = "FunctionExpression"; | ||||||
fn.ensureBlock(); | ||||||
fn.node.type = "FunctionExpression"; | ||||||
if (!noNewArrows) { | ||||||
const checkBinding = thisBinding | ||||||
? null | ||||||
: this.parentPath.scope.generateUidIdentifier("arrowCheckId"); | ||||||
: fn.scope.generateUidIdentifier("arrowCheckId"); | ||||||
if (checkBinding) { | ||||||
this.parentPath.scope.push({ | ||||||
fn.parentPath.scope.push({ | ||||||
id: checkBinding, | ||||||
init: objectExpression([]), | ||||||
}); | ||||||
} | ||||||
|
||||||
this.get("body").unshiftContainer( | ||||||
fn.get("body").unshiftContainer( | ||||||
"body", | ||||||
expressionStatement( | ||||||
callExpression(this.hub.addHelper("newArrowCheck"), [ | ||||||
|
@@ -178,10 +178,10 @@ export function arrowFunctionToExpression( | |||||
), | ||||||
); | ||||||
|
||||||
this.replaceWith( | ||||||
fn.replaceWith( | ||||||
callExpression( | ||||||
memberExpression( | ||||||
nameFunction(this, true) || this.node, | ||||||
nameFunction(this, true) || fn.node, | ||||||
identifier("bind"), | ||||||
), | ||||||
[checkBinding ? identifier(checkBinding.name) : thisExpression()], | ||||||
|
@@ -195,24 +195,44 @@ export function arrowFunctionToExpression( | |||||
* or "new.target", ensure that these references reference the parent environment around this function. | ||||||
*/ | ||||||
function hoistFunctionEnvironment( | ||||||
fnPath, | ||||||
fnPath: NodePath<t.Function>, | ||||||
// TODO(Babel 8): Consider defaulting to `false` for spec compliancy | ||||||
noNewArrows = true, | ||||||
allowInsertArrow = true, | ||||||
) { | ||||||
const thisEnvFn = fnPath.findParent(p => { | ||||||
): { thisBinding: string; fnPath: NodePath<t.Function> } { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add some comments about the return type? Especially we may have different |
||||||
let arrowParent; | ||||||
let thisEnvFn = fnPath.findParent(p => { | ||||||
if (p.isArrowFunctionExpression()) { | ||||||
arrowParent ??= p; | ||||||
return false; | ||||||
} | ||||||
return ( | ||||||
(p.isFunction() && !p.isArrowFunctionExpression()) || | ||||||
p.isProgram() || | ||||||
p.isClassProperty({ static: false }) | ||||||
p.isFunction() || p.isProgram() || p.isClassProperty({ static: false }) | ||||||
); | ||||||
}); | ||||||
const inConstructor = thisEnvFn?.node.kind === "constructor"; | ||||||
const inConstructor = thisEnvFn.isClassMethod({ kind: "constructor" }); | ||||||
|
||||||
if (thisEnvFn.isClassProperty()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
throw fnPath.buildCodeFrameError( | ||||||
"Unable to transform arrow inside class property", | ||||||
); | ||||||
if (arrowParent) { | ||||||
thisEnvFn = arrowParent; | ||||||
} else if (allowInsertArrow) { | ||||||
// It's safe to wrap this function in another and not hoist to the | ||||||
// top level because the 'this' binding is constant in class | ||||||
// properties (since 'super()' has already been called), so we don't | ||||||
// need to capture/reassign it at the top level. | ||||||
fnPath.replaceWith( | ||||||
callExpression( | ||||||
arrowFunctionExpression([], toExpression(fnPath.node)), | ||||||
[], | ||||||
), | ||||||
); | ||||||
thisEnvFn = fnPath.get("callee"); | ||||||
fnPath = thisEnvFn.get("body"); | ||||||
} else { | ||||||
throw fnPath.buildCodeFrameError( | ||||||
"Unable to transform arrow inside class property", | ||||||
); | ||||||
} | ||||||
} | ||||||
|
||||||
const { thisPaths, argumentsPaths, newTargetPaths, superProps, superCalls } = | ||||||
|
@@ -365,7 +385,7 @@ function hoistFunctionEnvironment( | |||||
} | ||||||
} | ||||||
|
||||||
return thisBinding; | ||||||
return { thisBinding, fnPath }; | ||||||
} | ||||||
|
||||||
function standardizeSuperProperty(superProp) { | ||||||
|
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.
Q: Can this check be strengthen to
path.isFunctionExpression()
?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.
Yes, currently
path
is guaranteed to be either aFunctionExpression
or aCallExpression