search

jitsi / lib-jitsi-meet

A low-level JS video API that allows adding a completely custom video experience to web apps.
Apache License 2.0
1.34k stars 1.11k forks source link

Add TypeScript definitions #1025

Open sapkra opened 4 years ago

sapkra commented 4 years ago

The common way to use types in JavaScript became TypeScript. The most projects dropped the flow support and migrated to TypeScript. I would really appreciate it if this project could either.

The best way would be to completely migrate to TypeScript but it would also be ok if a TypeScript definition file would exist to provide the typings.

saghul commented 4 years ago

I'd be really happy to migrate to TS, but I'll be realistic here: it's hard to justify, and this is no small codebase.

That said, adding TS definitions is a great start. Do you know of a way to automate the generation, at least to have a starting point, even if we have to manually adjust this later?

sapkra commented 4 years ago

I started with generating TypeScript definitions by using the jsdoc definitions but came across some errors. It would be very nice if you can help with that because I'm not experienced with jsdoc.

Here is my PR: #1027 The error logs are also attached.

saghul commented 4 years ago

I’ll take a look, thank you!

sapkra commented 4 years ago

btw. it would also be possible to migrate the codebase step by step to TypeScript by using allowJs inside the tsconfig.json. This would be the cleanest way and would save some time in the long run.

sapkra commented 4 years ago

For everybody who is still interested: I've added some generated type definitions to my published npm package @lyno/lib-jitsi-meet.

halvardssm commented 4 years ago

@saghul What is the current status of rewriting lib-jitsi-meet to Typescript? Where can I start if I want to contribute to this rewrite?

saghul commented 4 years ago

There is no progress. As I said in https://github.com/jitsi/lib-jitsi-meet/issues/1025#issuecomment-580615489 it'd be very hard to justify.

sapkra commented 4 years ago

@saghul I think it would not be that complicated if it would be possible to merge the migrations in small parts (for just a few files) very quickly. Because if those PRs are not merged quickly it would make it too complicated because the PR author has to rebase the PRs all the time to migrate changes and fix new conflicts.

Additionally to that the maintainers of this repo have to adapt to write typescript in the .ts files but I think this will also not be a big deal.

I've already migrated large projects to typescript and know how more stable the codebase is now and how much the workflow has improved.

halvardssm commented 4 years ago

I would be interrested in helping with the process of migrating to a full TS codebase, so if we can get this started, we can set up an overview and gradually migrate the files one by one. @saghul and @sapkra Let me know if this would be of interest and maybe we could TS the JitsiMeetExternalAPI interface during the upcoming weeks? I have been experimenting with Jitsi the last months, and one of the biggest hurdles for using it, was due to the lack of types and partial documentation, so I believe with a typed codebase, we will lower the entry bar a step to get more traction and usage of the library.

sapkra commented 4 years ago

My idea would be if I would get write access to this repo I could review the typescript PRs and merge them as soon as possible. These will not introduce breaking changes so there would be no problem IMO and the jitsi core team doesn't have to review all of those PRs. @saghul WDYT? We can also have a call about this topic, if you want?

knallfr0sch commented 3 years ago

I think not providing *.d.ts files is a great way to increase support costs and offering a lower-quality product, while not saving much work. TypeScript introduced the ability of creating declaration files from JSDoc (something this project already has!)

With TypeScript 3.7, TypeScript added support for generating .d.ts files from JavaScript using JSDoc syntax.

https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html

Hope this helps end-users infer the typings they need and/or convinces the Jitsi team to finally make the switch.

Also remember: You could also simply rename all *.jsfiles to *.ts. Valid JavaScript is valid TypeScript. Simply run the files through the TS compiler once. Then, gradually expand static typing with every code base change.

sapkra commented 3 years ago

I already tried generating them from jsdoc but the result isn't that good.

jomgapuz commented 3 years ago

A TypeScript Story: Day 1, too early to add it. Day 10, it's too late. 😂😬

garyhuntddn commented 3 years ago

Any thoughts on where we're going with this?

I went through the codebase a week or so ago and created a .d.ts by hand (it didn't take that long) and found a few jsdoc errors along the way that I intend to create PRs for.

We've a number of choices:

  1. attempt to maintain a .d.ts file independent of the codebase (like I've had to - but not a great way forward)
  2. improve the jsdoc so that we can use an automated tool

My preference would be option 2 and very happy to chip in and help out.

I'm also happy to share my .d.ts that I created manually so that we can use it as a benchmark. There are lots of types that aren't documented in the jsdoc that I was able to determine when working through the codebase by hand.

saghul commented 3 years ago

We can try to add the types file and keep improving it overtime. It doesn't need to be perfect day one.

Care to make a PR with it?

garyhuntddn commented 3 years ago

Sure - I did it as a quick exercise to see how possible it would be - so I removed all of the comments (I'll regret that) - but I can try and set something up that generates the automated and my one - and does a diff.

We'll want to publish it either as a separate NPM or within the existing install... I'll need to go look and see what the norm is - it might be index.d.ts.

Once I've transferred what I've got we can review in the PR.

Expect a PR sometime later in the week.

garyhuntddn commented 3 years ago

OK https://github.com/jitsi/lib-jitsi-meet/pull/1503 is now in place.

Very happy to work with others on this - it's been a slog to get the hand-crafted ones in place - but by doing so we now have an approximate baseline to work from.

In the PR I've put some notes about the tooling I've included to help make this a less painful experience.

Fuzzyma commented 3 years ago

@garyhuntddn you could add the typings as @typings/lib-jitsi-meet but that would somehow mean that it's independent of this project. The other possibility would be to generate a d.ts file automatically on every release and make it a no-brainer

Ferm9404 commented 3 years ago

Any progress on this? I need definitions for JitsiMeetExternalAPI

saghul commented 3 years ago

Any progress on this? I need definitions for JitsiMeetExternalAPI

That will not be covered by these, since JitsiMeetExternalAPI is not part of this project but of jitsi-meet.