Support the override keyword from TypeScript 4.3.
#630
Conversation
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-3.html#override-and-the---noimplicitoverride-flag This implementation is modeled on the `protected` keyword, which is similarly stripped from TypeScript function declarations. Fixes #629 and addresses preactjs/wmr#714
Codecov Report
@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 82.23% 82.12% -0.11%
==========================================
Files 56 56
Lines 5883 5651 -232
Branches 1330 1330
==========================================
- Hits 4838 4641 -197
+ Misses 762 727 -35
Partials 283 283
Continue to review full report at Codecov.
|
Benchmark resultsBefore this PR: 247.3 thousand lines per second Measured change: 0.3% slower (2.49% slower to 3.07% faster) |
There was a problem hiding this comment.
Hi @lgarron , thanks for the contribution! Unfortunately, I tested it out and am getting a parse error when I try it even on basic cases, so looks like it's not quite working end-to-end.
Usually the way that new syntax gets added to Sucrase is by porting babel-parser changes like in this PR: #567 . Sucrase's parser was originally forked from Babel's parser, so usually the Babel implementation is a good code sketch of how to fix it in Sucrase, though sometimes the code has diverged enough that it's easier to just write the change from scratch. I try to comb through all changes and port relevant ones from time to time, though I'm certainly also open to individual high-priority changes like the override keyword here. I looked through the Babel changelog and looks like the override implementation is here: babel/babel#13097 . I think the typescript/index.js changes are the most useful there, particularly updating calls to tsParseModifiers. Note that any error checking (e.g. this.raise) is out of scope since Sucrase doesn't try to be an error checker.
It would also be good to see some tests for this PR, which can go in test/typescript-test.ts. Feel free to use the Babel tests for inspiration, or write your own code snippets. Just testing some basic cases should be fine.
| @@ -46,6 +46,7 @@ const RESERVED_WORDS = new Set([ | |||
| "implements", | |||
| "interface", | |||
| "let", | |||
| "override", | |||
There was a problem hiding this comment.
override is actually not a reserved word in JS or TS, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#keywords . It's a "contextual keyword", so it can be used as a variable name, but also has meaning in certain contexts. So it actually shouldn't be added to this file.
| @@ -164,6 +164,7 @@ const types = { | |||
| _public: new KeywordTokenType("public"), | |||
| _private: new KeywordTokenType("private"), | |||
| _protected: new KeywordTokenType("protected"), | |||
| _override: new KeywordTokenType("override"), | |||
There was a problem hiding this comment.
I was going to say that it's weird there to be both a regular keyword and a contextual keyword for override, and ideally it would be only a contextual keyword because it's not a true language keyword. But I guess other modifiers use the same strategy (including non-keywords like readonly), so seems very reasonable to be consistent with that. FWIW, it feels fragile because the transformer won't necessarily know when to use this.tokens.matches1(tt._override) and when to use this.tokens.matchesContextual(ContextualKeyword._override), so maybe it's something that could be cleaned up in the future.
|
I just put up #636, which will cover this case in addition to the other TS 4.3 syntax. I'll close this PR and the corresponding task when that one is merged and released. |
|
I just released Sucrase 3.20.0 that has support for |
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-3.html#override-and-the---noimplicitoverride-flag
This implementation is modeled on the
protectedkeyword, which is similarly stripped from TypeScript function declarations.Fixes #629 and addresses preactjs/wmr#714