Skip to content

Conversation

@moretti
Copy link
Contributor

@moretti moretti commented Jun 7, 2023

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

januscla commented Jun 7, 2023

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

@atoppi
Copy link
Contributor

atoppi commented Jun 7, 2023

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

moretti commented Jun 7, 2023

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

atoppi commented Jun 7, 2023

@moretti stream is not supported anymore.

@moretti
Copy link
Contributor Author

moretti commented Jun 7, 2023

@atoppi Thanks, updated!

@moretti moretti force-pushed the update-ts-definition branch from 9a28a2c to 27f179d Compare June 7, 2023 12:32
@lminiero
Copy link
Member

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

@moretti
Copy link
Contributor Author

moretti commented Jun 13, 2023

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

@lminiero
Copy link
Member

Ack, merging then ✌️

@lminiero lminiero merged commit f9da1d1 into meetecho:master Jun 13, 2023
@moretti moretti deleted the update-ts-definition branch June 14, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants