meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.17k stars 2.47k forks source link

Update TypeScript signature of createOffer #3232

Closed moretti closed 1 year ago

moretti commented 1 year ago

This PR introduces an update to the TypeScript signature of the createOffer API method to include correct type annotations, aligning with the documentation provided on Janus API Documentation for initial offer creation and ICE restart.

I suggest considering the use of TSDoc to document the API and generate part of the JavaScript API documentation. Integrating it with Doxygen could improve codebase maintainability, readability, and automate the process of keeping the API documentation up-to-date.

januscla commented 1 year ago

Thanks for your contribution, @moretti! Please make sure you sign our CLA, as it's a required step before we can merge this.

atoppi commented 1 year ago

Thanks @moretti for the patch and the suggestions! In general it looks good to merge.

Even if not documented, the createOffer method still supports the deprecated media parameter though, so I'm not sure if it should be part of the annotations or not.

As a side note, if you have experience with annotations/documentations for js projects, I'll be glad to hear your opinion about the types enhancements for janode, since people discussing there seem to prefer type files over jsdoc/tsdoc.

moretti commented 1 year ago

@atoppi I see, I wasn't aware that the old API is still supported. In that case, it would be a good idea to mark the deprecated methods as such, so that editors supporting TypeScript can provide warnings when they are used.

Deprecated

Could you please confirm if the stream parameter is still supported? After upgrading to the latest version, it didn't seem to work for me anymore 🤔


Regarding the types enhancements for janode, it seems that the type definitions are .d.ts files with JSDoc annotations.

From my understanding, when documenting TypeScript and .d.ts files, it is recommended to use TSDoc as it aligns well with TypeScript's capabilities of inferring types. This allows for certain things like types to be omitted.

For JavaScript with JSDoc:

/**
 * @param {string} somebody - Somebody's name.
 */
function sayHello(somebody) {
    console.log(`Hello ${somebody}`);
}

For TypeScript with TSDoc:

/**
 * @param somebody - Somebody's name.
 */
function sayHello(somebody: string): void {
    console.log(`Hello ${somebody}`);
}
atoppi commented 1 year ago

@moretti stream is not supported anymore.

moretti commented 1 year ago

@atoppi Thanks, updated!

lminiero commented 1 year ago

@moretti unless you think there's more to add to this patch, it looks good to merge for me, thanks! :+1:

moretti commented 1 year ago

Hey @lminiero, I don't see any more additions needed for this patch. It's good to merge from my end. Thanks! 🙏

lminiero commented 1 year ago

Ack, merging then :v: