Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Support auto accessors with TypeScript annotations #15209
fix: Support auto accessors with TypeScript annotations #15209
Changes from 1 commit
0a40615
5f1d823
69726d9
f3fe423
d9764c0
4845f74
1080dd1
68312ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
TS will throw
Declarations with definite assignment assertions must also have type annotations.
, we can address that in a separate PR.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 seems that other errors are not reported.
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.
Yes, I only reported that unrecoverable error.
Actually initially I also implemented another one, but then I thought they didn't matter.
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.
Doesn't the
output.json
in parser tests only report recoverable errors unless one seterrorRecovery: false
? The unrecoverable error will be reported atoptions.throw
.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.
I mean currently the only errors reported are from unrecoverable errors. And the rest are illegal but normal parsing, I don't think it matters, so they haven't been implemented.
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.
Why is
accessor x?
an unrecoverable error? Can you elaborate?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.
https://babeljs.io/repl#?browsers=&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=MYGwhgzhAEDC0G8BQ1pmMAplA9gJ2gGIAPAfgG4kBfIA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=stage-2&prettier=false&targets=Node-0&version=7.20.4&externalPlugins=&assumptions=%7B%7D
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.
Although
tsc
parser is a loose parser, it does not necessarily mean it should not report errors. TheerrorRecovery
option enables Babel the same tsc functionality: report errors without crashing the parser.Generally the Babel parser does not report typing errors, such as "the call signature does not match the function interface": they are considered as "runtime errors" because these errors can only be reported from a typing system.
But for those errors that can be detected solely from AST, they are most static errors and thus they should be reported by the parser. We have reported similar errors, e.g. Babel throws "Private elements cannot have an accessibility modifier" for
Therefore I am not convinced the idea that "the rest are illegal but normal parsing". If TS would ever have a spec, they should be early errors and a parser should report them.
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.
My bad, by a better error message, I do mean that we can recover from this error and throw a recoverable error.
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.
I personally prefer to make unrecoverable errors recoverable, and normally parsed ts errors are not actively reported. (at least when no user requests these)
Because I believe all users will use tsc at the same time, and ts makes too many errors recoverable.
I've done this and the only error is from what you said.