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

Update TypeScript signature of createOffer #3232

Merged
merged 4 commits into from
Jun 13, 2023

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
Member

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
Member

atoppi commented Jun 7, 2023

@moretti stream is not supported anymore.

@moretti
Copy link
Contributor Author

moretti commented Jun 7, 2023

@atoppi Thanks, updated!

@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