-
Notifications
You must be signed in to change notification settings - Fork 12.8k
convert JSDoc typedef to type, issue 50644 #51430
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
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@microsoft-github-policy-service agree company="LinkedIn" |
1 similar comment
@microsoft-github-policy-service agree company="LinkedIn" |
be1417c
to
1281a38
Compare
943aa8d
to
970503c
Compare
@andrewbranch Would you take a look at this PR? Thanks. |
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 is a first past skim review; I think I'd need to think of some edge cases here. Probably @andrewbranch and @sandersn know more.
fc00ee7
to
9c4a249
Compare
It's too late now, but if you can avoid it, I'd appreciate merges from main and additional commits (instead of force pushing) as it's hard to follow what changed in the PR and what comments have been addressed. We squash on PR merge so no need to keep the PR clean. (See our contributing guide: https://github.com/microsoft/TypeScript/blob/main/CONTRIBUTING.md#force-pushing) |
d63d7d4
to
20eda98
Compare
Okay. Will do. |
@jakebailey @andrewbranch After I rebased yesterday, the newly added tests failed, which passed before. As I ran the debugger, the type alias declaration was successfully created. Then node.modifiers is passed into Line 1280 of src/compiler/visitorPublic.ts, then assignPositionsToNodeArray() of src/services/textChanges.ts, then eventually failed at Line 178 in the getPos() function of src/services/textChanges.ts. The value of node.modifiers is [pos: -1, end: -1, hasTrailingComma: false, transformFlags: 0]. Please see screenshots attached. Thank you very much for your help! |
I checked, and it's because you're passing in an empty array for modifiers to |
@jakebailey Thank you very much! The tests now passed. Please review again. |
@jakebailey @andrewbranch Would you take another look at this PR? Thanks. |
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.
Seems reasonable, I think? Still not an expert, but it seems about as concise as I can imagine.
@andrewbranch Would you take a look? Thank you very much. |
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.
Nice work, thanks @brendaHuang!
Fixes #50644
This PR converts JSDoc typedef to type. It converts two common use cases of @typedef as listed in the JSDoc documentation and other variations, such as nested object types and using @typedef to define primitive types.
Of the five test files, one one addresses the union type, and one primitive type.
The other three addresses object types (index signatures), including nested object types and object types that have comments attached to the properties.