Allow context type annotation on getters/setters #9641
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10410/ |
matt-tingen
force-pushed
the
8069-typescript-getter-setter-this
branch
from
b8adb6a
to
238598c
Compare
danez
added
pkg: parser
area: typescript
PR: Spec Compliance 👓
| method.params[0].type === "Identifier" && | ||
| method.params[0].name === "this" | ||
| ) { | ||
| this.expectPlugin("typescript"); |
There was a problem hiding this comment.
When the first parameter is (EDIT: I was wrong)this the new implementation doesn't check the parameters count because the } else if (method.params.length !== paramCount) { branch is skipped:
//get prop(this: {}, foo, bar) {}If we parsed this as a function parameter, then this plugin is already enabled (no need to assert). I would prefer to have as few as possible TypeScript-specific logic in the parser/* files. WDYT about extractions method.kind === "get" ? 0 : 1 to a new getAccessorsExpectedParamCount(method) function and overwrite it in the typescript plugin?
There was a problem hiding this comment.
method.params.length === paramCount + 1 wouldn't be true in that case I guess?
There was a problem hiding this comment.
@nicolo-ribaudo I have pushed a change for getGetterSetterExpectedParamCount. Thanks for suggesting the separation. I hadn't noticed I had the first usage of this.expectPlugin("typescript").
At first, it felt a little odd that the error messages directly reference the required number of params despite that number being abstracted away. I considered extracting a checkGetterSetterExpectedParamCount method instead, but didn't find a good way to do so without duplicating logic in the TypeScript plugin. However, I found that there's less of a dissonance if I look at it as the error messages being based on target language and getGetterSetterExpectedParamCount being based on the source language, so I'm on board now 😄
|
Thanks! |
|
My pleasure! Thanks for the quick reviews! |
This is a hold-over until babel#9641 is released.
This is a hold-over until babel#9641 is released.
* master: (58 commits) Remove dependency on home-or-tmp package (babel#9678) [proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys (babel#9628) Partial application plugin (babel#9474) Private Static Class Methods (Stage 3) (babel#9446) gulp-uglify@3.0.2 rollup@1.6.0 eslint@5.15.1 jest@24.5.0 regexpu-core@4.5.4 Remove input and length from state (babel#9646) Switch from rollup-stream to rollup and update deps (babel#9640) System modules - Hoist classes like other variables (babel#9639) fix: Don't transpile ES2018 symbol properties (babel#9650) Add WarningsToErrorsPlugin to webpack to avoid missing build problems on CI (babel#9647) Update regexpu-core dependency (babel#9642) Add placeholders support to @babel/types and @babel/generator (babel#9542) Generate plugins file Make babel-standalone an ESModule and enable flow (babel#9025) Reorganize token types and use a map for them (babel#9645) [TS] Allow context type annotation on getters/setters (babel#9641) ...
|
Hm, is this part of the latest 7.3.3 release? Still getting:
errors on this? Might be our setup, but yeah, just wanted to ask :) Or maybe it was just solved for classes? I am using it on plain objects: {
foo(this: Foo) {
}
} |
|
@christianalfoni This was released in 7.4.0. It's still working for me in the REPL. |
|
Oh, hm, it seems that @babel/preset-typescript is not keeping up with latest versions? |
|
@christianalfoni I'm not sure how the auxiliary packages are versioned. Most of them seem to keep in sync with core, but some may not need any changes for a given core bump. |
|
We only publish updated packages when they are actually changed. One of these commands should work: npm update @babel/plugin-transform-typescript
yarn upgrade @babel/plugin-transform-typescript |
lock
bot
added
the
outdated
Typescript allows adding type annotations to the context (
this) for functions, includinggetters andsetters. Presently, the parser errors out when such an annotation is present on a getter or setter because it expects exactly 0 or 1 parameter, respectively.This change causes the parser to allow an extra first parameter to getters and setters when using the
typescriptplugin and the parameter'snameisthis.babel-plugin-transform-typescriptalready strips thethis: ...annotation from functions, including getters and setters, but I added a test to verify this in addition to the parser test.