Skip to content
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

Fix rest parameters indexing with TypeScript 'this parameter' #9714

Merged

Conversation

BenoitZugmeyer
Copy link
Contributor

@BenoitZugmeyer BenoitZugmeyer commented Mar 20, 2019

Q                       A
Fixed Issues? Fixes #8840
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

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-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.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 20, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10654/

@BenoitZugmeyer BenoitZugmeyer force-pushed the fix/ts-this-param-with-rest-params branch from b8c535f to 4f0fda5 Compare March 20, 2019 13:33
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
@BenoitZugmeyer BenoitZugmeyer force-pushed the fix/ts-this-param-with-rest-params branch from 4f0fda5 to aa64170 Compare April 2, 2019 13:35
@BenoitZugmeyer
Copy link
Contributor Author

Is there anything else I can do to make this PR right?

@alamothe
Copy link

Any updates?

@canpoyrazoglu
Copy link

I'm affected by this bug too, and there's this PR that fixes the issue. Any updates on when this would be merged?

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍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.

REPL

* improve performance by checking the parameter list length before
accessing it
* simplify the test a bit by using the `isIdentifier` second
parameter
@BenoitZugmeyer
Copy link
Contributor Author

Thanks for the review! I processed your comments, and merged master to make sure the updated tests are still passing.

@@ -2,6 +2,7 @@
"plugins": [
"proposal-class-properties",
"external-helpers",
"syntax-typescript",
"syntax-flow",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that this works, flow and typescript shouldn't be compatible.

@nicolo-ribaudo nicolo-ribaudo added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jan 13, 2020
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" })) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be checking that there's a typeAnnotation attached to this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 18, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[[email protected]] Combination of 'this' parameter and rest parameters goes wrong
8 participants