Conversation
ajafff
left a comment
There was a problem hiding this comment.
Do you plan on adding the corresponding updateJSDocXXX functions?
src/compiler/factory.ts
Outdated
| } | ||
|
|
||
| export function createJSDocUnknownTag() { | ||
| return createSynthesizedNode(SyntaxKind.JSDocTag) as JSDocUnknownTag; |
There was a problem hiding this comment.
is this function even necessary? createJSDocTag should be used instead
There was a problem hiding this comment.
JSDocUnknownTag haven't gov the tagName
There was a problem hiding this comment.
it has, it extends JSDocTag and therefore contains tagName and comment
src/compiler/factory.ts
Outdated
| /* @internal */ | ||
| function createJSDocTag<T extends JSDocTag>(kind: T["kind"], tagName: string): T { | ||
| const node = createSynthesizedNode(kind) as T; | ||
| node.tagName = createIdentifier(tagName); |
There was a problem hiding this comment.
this is missing an assignment to node.comment
There was a problem hiding this comment.
That not my fault,
/cc: @DanielRosenwasser
src/compiler/factory.ts
Outdated
| return createSynthesizedNode(SyntaxKind.JSDocTag) as JSDocUnknownTag; | ||
| } | ||
|
|
||
| export function createJSDocAugmentsTag(tagName: string, classExpression: ExpressionWithTypeArguments & { expression: Identifier | PropertyAccessEntityNameExpression }, comment?: string) { |
There was a problem hiding this comment.
prefer classExpression: JSDocAugmentsTag['class']
src/compiler/factory.ts
Outdated
| export function createJSDocEnumTag(tagName: string, typeExpression?: JSDocTypeExpression, comment?: string) { | ||
| const node = createJSDocTag<JSDocEnumTag>(SyntaxKind.JSDocEnumTag, tagName); | ||
| node.typeExpression = typeExpression; | ||
| node.comment = comment; |
There was a problem hiding this comment.
this assignment should be done in createJSDocTag
src/compiler/factory.ts
Outdated
| return node; | ||
| } | ||
|
|
||
| export function createJSDocPropertyLikeTag<T extends JSDocPropertyLikeTag>(kind: T["kind"], tagName: Identifier, name: EntityName, isNameFirst: boolean, isBracketed: boolean, typeExpression?: JSDocTypeExpression, comment?: string) { |
There was a problem hiding this comment.
should this helper function really be exported?
src/compiler/factory.ts
Outdated
| return createJSDocTagWithTypeExpression<JSDocThisTag>(SyntaxKind.JSDocThisTag, tagName, typeExpression, comment); | ||
| } | ||
|
|
||
| export function createJSDocTemplateTag(tagName: string, typeParameters: NodeArray<TypeParameterDeclaration>, constraint?: JSDocTypeExpression, comment?: string) { |
There was a problem hiding this comment.
consider using ReadonlyArray over NodeArray for easier to use API. in the function body you can then use toNodeArray or asNodeArray to convert it as necessary
src/compiler/factory.ts
Outdated
|
|
||
| /* @internal */ | ||
| function createJSDocTag<T extends JSDocTag>(kind: T["kind"], tagName: string): T { | ||
| export function createJSDocAugmentsTag(tagName: string, classExpression: JSDocAugmentsTag["class"], comment?: string) { |
There was a problem hiding this comment.
tagName should be limited to "augments" | "extends"
src/compiler/factory.ts
Outdated
| return node; | ||
| } | ||
|
|
||
| export function createJSDocClassTag(tagName: string, comment?: string) { |
There was a problem hiding this comment.
is the tagName parameter necessary? it should always be "class"
src/compiler/factory.ts
Outdated
|
|
||
| function createJSDocTagWithTypeExpression<T extends JSDocTag & { typeExpression?: TypeNode }>(kind: T["kind"], tagName: string, typeExpression?: TypeNode, comment?: string) { | ||
| const node = createJSDocTag<T>(kind, tagName, comment); | ||
| node.typeExpression = typeExpression; |
There was a problem hiding this comment.
AFAIK the factory functions should set properties in the same order as the parser to avoid multiple shapes of the same object.
Looking at parseReturnTag as an example: it sets tagName, typeExpression and comment (in parseTag function).
Maybe you want to change the parser to initialize the properties in the same order as the newly added factories (comment before typeExpression and so on)
There was a problem hiding this comment.
In fact, I also have concerns about this, I decide to remove this helper function.😢
|
⬆️ |
|
🙋🏻♂️ |
|
@DanielRosenwasser @RyanCavanaugh @rbuckton Please take a look at this change as the issue is not marked as help wanted and is suggestion so not sure if we agreed/discussed if this is right approach. |
|
@sandersn @DanielRosenwasser @RyanCavanaugh any update? |
|
Any update?@DanielRosenwasser @RyanCavanaugh @rbuckton @sandersn |
|
Could we push this down (after 3.9 release)? it's useful for the code generator. |
| return createJSDocPropertyLikeTag<JSDocPropertyTag>(SyntaxKind.JSDocPropertyTag, "param", typeExpression, name, isNameFirst, isBracketed, comment); | ||
| } | ||
|
|
||
| export function createJSDocParameterTag(typeExpression: JSDocTypeExpression | undefined, name: EntityName, isNameFirst: boolean, isBracketed: boolean, comment?: string) { |
There was a problem hiding this comment.
why have both createJSDocParameterTag and createJSDocParamTag. The only real difference is that this sets isNameFirst but createJSDocParamTag doesn't.
Naively, it feels to me like we should only have one, which uses this code but is named createJSDocParamTag.
There was a problem hiding this comment.
Well, except that we shouldn't break the API unnecessarily. Ugh. Can you add documentation saying that createJSDocParamTag is deprecated and that this is the preferred function now?
src/compiler/factoryPublic.ts
Outdated
| return tag; | ||
| } | ||
|
|
||
| export function createJSDocImplementTag(classExpression: JSDocImplementsTag["class"], comment?: string) { |
There was a problem hiding this comment.
| export function createJSDocImplementTag(classExpression: JSDocImplementsTag["class"], comment?: string) { | |
| export function createJSDocImplementsTag(classExpression: JSDocImplementsTag["class"], comment?: string) { | |
|
Nope, please feel free to this. |
|
Squash those comments please if someone going to merge. |
|
@typescript-bot pack this. |
|
Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
up plz :XD |
|
Unfortunately there is some overlap between this PR and #35282 as I also have all of these methods on the new |
Fixes #29341