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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly transpile when default parameter initializer references binding in rest pattern #11326
Conversation
Fix issue 11281. Transform parameters with default initializers that have ids that are also in a parameter with a rest element. Before, these parameters were not transformed.
@@ -213,8 +218,67 @@ export default declare((api, opts) => { | |||
// function a({ b, ...c }) {} | |||
Function(path) { | |||
const params = path.get("params"); | |||
for (let i = params.length - 1; i >= 0; i--) { | |||
replaceRestElement(params[i].parentPath, params[i]); | |||
const paramHasRestElement = []; |
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.
It suffices to model paramHasRestElement
as Set<number>
and to only include the index of which has rest element.
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.
Changed. I used an array of booleans instead of a set since the code was about the same length and I figured array access would be (very marginally) faster than set access.
Previously, f({...R}, f = R => R) would be incorrectly transformed.
Any more feedback? |
Checking the RHS of an assignment pattern/checking the parent of an identifier node fails on cases like "({...R}, a = f(R))" or "({...R}, {[R.key]: a = 42})". Also refactor tests by removing unecessary tests and separating "should transform" from "should not transform" tests.
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.
Awesome!
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.
Thank you!
Note: If the title seems familiar, it is because I copied it from #11317, which I closed because I rewrote the code to a large degree, so the commit history was useless. Third time is the charm 馃檭
Fixes the bug where
({ a, ...R }, b = R, c = a) => {}
was not transpiled correctly iftransform-function-parameters
was not enabled.I fixed it by modifying
convertFunctionParams
to accept a function to transform rest elements.