-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Readonly support for jsdoc #35790
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
Readonly support for jsdoc #35790
Conversation
Add ctor function test Add some notes and rename variable
error(right, Diagnostics.Cannot_assign_to_0_because_it_is_a_read_only_property, idText(right)); | ||
return errorType; | ||
} | ||
if (isAssignmentToReadonlyEntity(node as Expression, prop, assignmentKind)) { |
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 collapsed these three checks into one function because they are always called together.
const isLocalPropertyDeclaration = ctor.parent === symbol.valueDeclaration.parent; | ||
const isLocalParameterProperty = ctor === symbol.valueDeclaration.parent; | ||
const isLocalThisPropertyAssignment = isAssignmentDeclaration && symbol.parent?.valueDeclaration === ctor.parent; | ||
const isLocalThisPropertyAssignmentConstructorFunction = isAssignmentDeclaration && symbol.parent?.valueDeclaration === ctor; |
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.
This one is this.x = whatever
inside a JS constructor function?
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.
Yep. Any suggestions for making the variable name clearer?
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.
- isLocalThisPropertyAssignmentConstructorFunction
+ isLocalThisPropertyAssignmentInConstructorFunction
?
const isAssignmentDeclaration = isBinaryExpression(symbol.valueDeclaration); | ||
const isLocalPropertyDeclaration = ctor.parent === symbol.valueDeclaration.parent; | ||
const isLocalParameterProperty = ctor === symbol.valueDeclaration.parent; | ||
const isLocalThisPropertyAssignment = isAssignmentDeclaration && symbol.parent?.valueDeclaration === ctor.parent; |
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.
This would be true inside any method, but we’ve already ensured we’re in a constructor 👍
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.
How's the user suite look?
@typescript-bot user test this
@typescript-bot user test this |
@weswigham Looks like the bot doesn't pay attention to sign off comments. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
Results are the same as yesterday's |
Add
@readonly
to jsdoc; works the same way readonly does in TS.Fixes #17233