Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

JSDoc overload tag#51234

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
sandersn merged 8 commits intomicrosoft:mainfromapendua:js-doc-overload-tag
Dec 13, 2022
Merged

Conversation

apendua
Copy link
Contributor

Fixes#25590

This PR replaces an earlier attempt:#50789

Since there was no final consensus, I ended up using a new@overload tag to ensure backwards compatibility, e.g.

/** *@overload *@param {number} x *@returns {'number'} *//** *@overload *@param {string} x *@returns {'string'} *//** *@overload *@param {boolean} x *@returns {'boolean'} *//** *@param {unknown} x *@returns {string} */functiontypeName(x){returntypeofx;}

I would be happy to modify the implementation if someone suggests a better approach.

Mlocik97, anuraghazra, jespertheend, SchroederSteffen, GoToLoop, RRafael-Dev, and MAZ01001 reacted with thumbs up emojibajtos, jespertheend, ackvf, GoToLoop, and mackuba reacted with hooray emojibajtos, silane, jespertheend, and GoToLoop reacted with heart emojijespertheend and GoToLoop reacted with rocket emoji
@typescript-bottypescript-bot added the For Backlog BugPRs that fix a backlog bug labelOct 19, 2022
@apenduaapendua mentioned this pull requestOct 19, 2022
@apendua
Copy link
ContributorAuthor

@microsoft-github-policy-service agree

@sandersnsandersn self-assigned thisNov 15, 2022
Copy link
Member

@sandersnsandersn left a comment

Choose a reason for hiding this comment

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

This looks great. Clever re-use of existing infrastructure for@callback. Just a few questions and requests for additional tests. I also want to think about the design a bit more, but so far I think it's solid.

=== tests/cases/compiler/jsFileFunctionOverloads.js ===
/**
* @overload
* @param {number} x
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what happens if you find-all-ref or rename the overload parameters. In Typescript, they are not connected to the implementation signatures and don't rename.

Can you add two tests in cases/fourslash, one that renames an overload param and one that find-all-refs it, and shows that overload params are not connected to the implementation? There should be existing tests for@param that you can start from.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I only tried this in VSCode, and it seems like your assumptions are correct, i.e. the overload parameters are generally ignored on bothfind-all-ref andrename operations.

I would be happy to add those tests, but can you please point me to an example where a similar test was already implemented? I am fairly new to this codebase and navigating around (especially within tests) is still challenging 😄

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I've added tests forfind-all-ref andrename as requested.

if (node.tags) {
for (const tag of node.tags) {
if (isJSDocOverloadTag(tag)) {
result.push(getSignatureFromDeclaration(tag.typeExpression));
Copy link
Member

Choose a reason for hiding this comment

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

Operations that go from a signature back to a declaration might fail if the signature is a JSDocSignature. But (1) JSDocCallbackTag already exists, and has shaken out those bugs. (2) I can't think of any specific places; the only new ones would be ones that operate on symbols withmultiple signatures.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Is there anything in particular here that you would like me to look into?

Comment on lines +6 to +7
*/
/**
Copy link
Member

Choose a reason for hiding this comment

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

what happens if all the@overload tags are in one comment? Does that work too?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, actually it does. At least the examples I checked locally seem to work properly. Do you think we should be adding this as a test case?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Just in case, I've also added another variant of tests, which tries to put all@overload tags in a single comment. It seems like things get tricky (unsurprisingly) if you also want to use@template tag, as you can only have a single "scope" for template parameters per JSDoc comment. This problem is not specific to@overload tag though, and I think it should be good enough for now if it's covered in documentation properly. Perhaps this could be improved in the future if we allow@template to be parsed as part ofJSDocSignature?

sandersn and GoToLoop reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it's good enough for now.

apendua reacted with hooray emoji

/**
* @overload
* @this {Example<'number'>}
Copy link
Member

Choose a reason for hiding this comment

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

I don't seethis in the type. Does this actually work?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thank you for pointing this out! I forgot thatJSDocSignature does not support@this tag, but it seems like@param {...} this works well. I've just pushed a fix.

@sandersn
Copy link
Member

I did a quick grep of JS code (all the JS that gets installed when installing the dependencies of every package on Definitely Typed). This found one existing use of@overload, in the AWS SDK. The use is similar to this PR, except that it's used on functions with a single signature, and also on classes. Can you add some tests that reflect that use? They don't have to behave the way that the authors meant, they just need to not crash. =)

Here are the interesting cases I've found so far:

callable constructor function

/**   *@overload Endpoint(endpoint)   *   Constructs a new endpoint given an endpoint URL. If the   *   URL omits a protocol (http or https), the default protocol   *   set in the global {AWS.config} will be used.   *@param endpoint [String] the URL to construct an endpoint from   */  constructor:functionEndpoint(endpoint,config){

callable constructor function with overloads

Note that the second overload has zero param tags, just@option and@example.

/**   * A credentials object can be created using positional arguments or an options   * hash.   *   *@overload AWS.Credentials(accessKeyId, secretAccessKey, sessionToken=null)   *   Creates a Credentials object with a given set of credential information   *   as positional arguments.   *@param accessKeyId [String] the AWS access key ID   *@param secretAccessKey [String] the AWS secret access key   *@param sessionToken [String] the optional AWS session token   *@example Create a credentials object with AWS credentials   *     var creds = new AWS.Credentials('akid', 'secret', 'session');   *@overload AWS.Credentials(options)   *   Creates a Credentials object with a given set of credential information   *   as an options hash.   *@option options accessKeyId [String] the AWS access key ID   *@option options secretAccessKey [String] the AWS secret access key   *@option options sessionToken [String] the optional AWS session token   *@example Create a credentials object with AWS credentials   *     var creds = new AWS.Credentials({   *       accessKeyId: 'akid', secretAccessKey: 'secret', sessionToken: 'session'   *     });   */  constructor:functionCredentials(){

The associated hand-written overload set:

constructor(options:CredentialsOptions);constructor(accessKeyId: string,secretAccessKey: string,sessionToken?: string);

single signature with split return types

There's a lot going on here that would be good to test:

  • function in object literal property assignment
  • double return tags; I think this is the 'overload' part
  • @callback inline type definition impllicitly covering the third parameter

Again, this doesn't need to have the correct semantics, it just needs to not crash.

    /**     * @overload getSynthesizeSpeechUrl(params = {}, [expires = 3600], [callback])     *   Generate a presigned url for {AWS.Polly.synthesizeSpeech}.     *   @note You must ensure that you have static or previously resolved     *     credentials if you call this method synchronously (with no callback),     *     otherwise it may not properly sign the request. If you cannot guarantee     *     this (you are using an asynchronous credential provider, i.e., EC2     *     IAM roles), you should always call this method with an asynchronous     *     callback.     *   @param params [map] parameters to pass to the operation. See the {AWS.Polly.synthesizeSpeech}     *     operation for the expected operation parameters.     *   @param expires [Integer] (3600) the number of seconds to expire the pre-signed URL operation in.     *     Defaults to 1 hour.     *   @return [string] if called synchronously (with no callback), returns the signed URL.     *   @return [null] nothing is returned if a callback is provided.     *   @callback callback function (err, url)     *     If a callback is supplied, it is called when a signed URL has been generated.     *     @param err [Error] the error object returned from the presigner.     *     @param url [String] the signed URL.     *   @see AWS.Polly.synthesizeSpeech     */    getSynthesizeSpeechUrl: function getSynthesizeSpeechUrl(params, expires, callback) {

Here's the hand-created .d.ts overload set for comparison:

getSynthesizeSpeechUrl(params:Polly.Types.SynthesizeSpeechInput,error:number,callback:(err:AWSError,url:string)=>void):void;getSynthesizeSpeechUrl(params:Polly.Types.SynthesizeSpeechInput,callback:(err:AWSError,url:string)=>void):void;getSynthesizeSpeechUrl(params:Polly.Types.SynthesizeSpeechInput,expires?: number): string;

@apendua
Copy link
ContributorAuthor

@sandersn

I did a quick grep of JS code (all the JS that gets installed when installing the dependencies of every package on Definitely Typed).

Thank you for taking the effort to find these examples!

They don't have to behave the way that the authors meant, they just need to not crash. =)

Sure, I will try to add the tests for the use cases that you listed.

@apendua
Copy link
ContributorAuthor

@sandersn I pushed some additional tests and also used this opportunity to resolve some conflicts. It would be great if you could have a second look 😄

// @declaration: true
// @filename: jsFileMethodOverloads2.js

// Also works if all @overload tags are combined in one comment.
Copy link
Member

Choose a reason for hiding this comment

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

👍


/**
* @overload
* @param {Example<number>} this
Copy link
Member

Choose a reason for hiding this comment

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

huh. I had no idea this worked.

apendua reacted with laugh emoji
@apendua
Copy link
ContributorAuthor

@sandersn Thank you for the review and for the positive feedback!

@sandersnsandersn merged commite4816ed intomicrosoft:mainDec 13, 2022
sandersn added a commit to sandersn/TypeScript that referenced this pull requestJan 28, 2023
Also, make@overload work like other jsdoc tags: only the last jsdoc tagbefore a declaration is used. That means all overload tags need to be inone tag:```js/** *@overload *@return {number} * *@overload *@return {string} */```function f() { return "1" }This no longer works:```js/** *@overload *@return {number} *//** *@overload *@return {string} */function f() { return "2" }```Fixesmicrosoft#51234
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@sandersnsandersnsandersn approved these changes

Assignees

@sandersnsandersn

Labels
For Backlog BugPRs that fix a backlog bug
Projects
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In JS, jsdoc should be able to declare functions as overloaded
3 participants
@apendua@sandersn@typescript-bot

[8]ページ先頭

©2009-2025 Movatter.jp