Skip to content
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

expose jsdoc factory #29539

Merged
merged 13 commits into from
May 6, 2020
Merged

expose jsdoc factory #29539

merged 13 commits into from
May 6, 2020

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Jan 23, 2019

Fixes #29341

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan on adding the corresponding updateJSDocXXX functions?

function createJSDocTag<T extends JSDocTag>(kind: T["kind"], tagName: string): T {
const node = createSynthesizedNode(kind) as T;
node.tagName = createIdentifier(tagName);
return node;
}

export function createJSDocUnknownTag() {
return createSynthesizedNode(SyntaxKind.JSDocTag) as JSDocUnknownTag;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this function even necessary? createJSDocTag should be used instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSDocUnknownTag haven't gov the tagName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has, it extends JSDocTag and therefore contains tagName and comment

export function createJSDocComment(comment?: string | undefined, tags?: NodeArray<JSDocTag> | undefined) {
const node = createSynthesizedNode(SyntaxKind.JSDocComment) as JSDoc;
node.comment = comment;
node.tags = tags;
return node;
}

/* @internal */
function createJSDocTag<T extends JSDocTag>(kind: T["kind"], tagName: string): T {
const node = createSynthesizedNode(kind) as T;
node.tagName = createIdentifier(tagName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing an assignment to node.comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That not my fault,
/cc: @DanielRosenwasser

return createSynthesizedNode(SyntaxKind.JSDocTag) as JSDocUnknownTag;
}

export function createJSDocAugmentsTag(tagName: string, classExpression: ExpressionWithTypeArguments & { expression: Identifier | PropertyAccessEntityNameExpression }, comment?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer classExpression: JSDocAugmentsTag['class']

export function createJSDocEnumTag(tagName: string, typeExpression?: JSDocTypeExpression, comment?: string) {
const node = createJSDocTag<JSDocEnumTag>(SyntaxKind.JSDocEnumTag, tagName);
node.typeExpression = typeExpression;
node.comment = comment;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assignment should be done in createJSDocTag

return node;
}

export function createJSDocPropertyLikeTag<T extends JSDocPropertyLikeTag>(kind: T["kind"], tagName: Identifier, name: EntityName, isNameFirst: boolean, isBracketed: boolean, typeExpression?: JSDocTypeExpression, comment?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this helper function really be exported?

return createJSDocTagWithTypeExpression<JSDocThisTag>(SyntaxKind.JSDocThisTag, tagName, typeExpression, comment);
}

export function createJSDocTemplateTag(tagName: string, typeParameters: NodeArray<TypeParameterDeclaration>, constraint?: JSDocTypeExpression, comment?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export function createJSDocComment(comment?: string | undefined, tags?: NodeArray<JSDocTag> | undefined) {
const node = createSynthesizedNode(SyntaxKind.JSDocComment) as JSDoc;
node.comment = comment;
node.tags = tags;
return node;
}

/* @internal */
function createJSDocTag<T extends JSDocTag>(kind: T["kind"], tagName: string): T {
export function createJSDocAugmentsTag(tagName: string, classExpression: JSDocAugmentsTag["class"], comment?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tagName should be limited to "augments" | "extends"

return node;
}

export function createJSDocClassTag(tagName: string, comment?: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the tagName parameter necessary? it should always be "class"


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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@Kingwl Kingwl Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I also have concerns about this, I decide to remove this helper function.😢

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 4, 2019

⬆️

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 21, 2019

🙋🏻‍♂️

@sheetalkamat
Copy link
Member

@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.

@xiaoxiangmoe
Copy link
Contributor

xiaoxiangmoe commented Nov 24, 2019

@sandersn @DanielRosenwasser @RyanCavanaugh any update?

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@sandersn sandersn added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug and removed For Backlog Bug PRs that fix a backlog bug labels Mar 26, 2020
@xiaoxiangmoe
Copy link
Contributor

xiaoxiangmoe commented Apr 14, 2020

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 14, 2020

Could we push this down (after 3.9 release)? it's useful for the code generator.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a fine idea to me, although I'll let @rbuckton have veto power.

However, I don't think we should ship this until 4.0 since we're in 3.9 beta. @Kingwl are you OK with this, or did I misinterpret your comment?

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return tag;
}

export function createJSDocImplementTag(classExpression: JSDocImplementsTag["class"], comment?: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function createJSDocImplementTag(classExpression: JSDocImplementsTag["class"], comment?: string) {
export function createJSDocImplementsTag(classExpression: JSDocImplementsTag["class"], comment?: string) {

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 16, 2020

Nope, please feel free to this.

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 16, 2020

Squash those comments please if someone going to merge.

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 17, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 17, 2020

Heya @Kingwl, I've started to run the tarball bundle task on this PR at e577f7e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 17, 2020

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/71483/artifacts?artifactName=tgz&fileId=3D437B95C52938407C7DBC39328F88EB27ABDA3C5150703D9DD87AD0F01F32E602&fileName=/typescript-3.9.0-insiders.20200417.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 26, 2020

up plz :XD

@sandersn sandersn merged commit e66ce87 into microsoft:master May 6, 2020
@Kingwl Kingwl deleted the expose-jsdoc-factory branch May 7, 2020 00:46
@rbuckton
Copy link
Member

rbuckton commented May 7, 2020

Unfortunately there is some overlap between this PR and #35282 as I also have all of these methods on the new NodeFactory API and I'm deprecating any factory-like methods exposed directly on the ts namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Expose jsdoc factory API
7 participants