Fix rest parameters indexing with TypeScript 'this parameter'#9714
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10654/ |
b8c535f to
4f0fda5
Compare
The TypeScript [this parameter][0] is a fake parameter and should not be taken into account when counting the function parameters. This patch skips it by using the condition taken from the `transform-typescript` plugin. Note: since the `transform-typescript` plugin is removing this kind of parameter, including it before the `transform-parameters` plugin solves the issue. This patch fixes the issue in other cases. [0]: https://www.typescriptlang.org/docs/handbook/functions.html#this-parameters
4f0fda5 to
aa64170
Compare
|
Is there anything else I can do to make this PR right? |
|
Any updates? |
|
I'm affected by this bug too, and there's this PR that fixes the issue. Any updates on when this would be merged? |
JLHwung
left a comment
There was a problem hiding this comment.
👍for the code despite of nits.
Note that there is another problem related to the fake this parameter: When transform-reserve-keywords and syntax-typescript are enabled. The fake this parameter should be preserved, however it will be renamed as _this in current master.
…-with-rest-params
* improve performance by checking the parameter list length before accessing it * simplify the test a bit by using the `isIdentifier` second parameter
|
Thanks for the review! I processed your comments, and merged master to make sure the updated tests are still passing. |
| "proposal-class-properties", | ||
| "external-helpers", | ||
| "syntax-typescript", | ||
| "syntax-flow", |
There was a problem hiding this comment.
I'm surprised that this works, flow and typescript shouldn't be compatible.
| function getParamsCount(node) { | ||
| let count = node.params.length; | ||
| // skip the first parameter if it is a TypeScript 'this parameter' | ||
| if (count > 0 && t.isIdentifier(node.params[0], { name: "this" })) { |
There was a problem hiding this comment.
Should we be checking that there's a typeAnnotation attached to this?
There was a problem hiding this comment.
I don't think that it is needed: this is not a valid binding identifier, so it can't be anything else other than TypeScript.
The TypeScript "this parameter" is a fake parameter and should not be taken into account when counting the function parameters. This patch skips it by using the condition taken from the
transform-typescriptplugin.Note: since the
transform-typescriptplugin is removing this kind of parameter, including it before thetransform-parametersplugin solves the issue. This patch fixes the issue in other cases.