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

Type defs #5

Draft
wants to merge 25 commits into
base: master
Choose a base branch
from
Draft

Type defs #5

wants to merge 25 commits into from

Conversation

augustblack
Copy link
Contributor

Here is a potential first start at typescript definitions for janode.

I did my best to create the defs without introducing any extra tooling.
All defs live under ./types. However, it does require modifying the package.json
I believe that since you require a minimum of node 14, this should be fine.
To use with typescript, one will need a recent 4.7 and set ["moduleResolution": "nodenext"] in the tsconfig.json

I'd be happy to do my best to maintain the types moving forward

@atoppi
Copy link
Member

atoppi commented Apr 11, 2022

Thanks @augustblack for the huge work!
I'll take a look in the next few days.

@augustblack
Copy link
Contributor Author

Thanks @augustblack for the huge work! I'll take a look in the next few days.

Alright, let me know. Happy to make adjustments if needed. I'm concerned that the way I have it set up requires a nightly build of typescript. Only the most recent typescript will recognize the "import": { "types" :... } format in package.json

Maybe there is a better way to organize the definitions for less bleeding-edge implementations.

This is my first time trying to package ts definitions.

@atoppi
Copy link
Member

atoppi commented Apr 13, 2022

I'm concerned that the way I have it set up requires a nightly build of typescript.

I agree, if a not-stable version of TS is needed I guess this can not be considered for merging now.

This is my first time trying to package ts definitions.

Me too :-) Need to gather some data before reviewing / enhancing.

I'm converting the PR to "draft" since I suspect more work is definitely needed.

@atoppi atoppi marked this pull request as draft April 13, 2022 09:55
@atoppi atoppi added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed labels Apr 13, 2022
@augustblack
Copy link
Contributor Author

augustblack commented Apr 13, 2022

I'm concerned that the way I have it set up requires a nightly build of typescript.

I agree, if a not-stable version of TS is needed I guess this can not be considered for merging now.

This is my first time trying to package ts definitions.

Me too :-) Need to gather some data before reviewing / enhancing.

I'm converting the PR to "draft" since I suspect more work is definitely needed.

No problem. Fwiw. the definitions work great and I don't suspect it will be long before TS 4.7 is released.

AFACT, if we could pull the defs into a single .d.ts , then we shouldn't have issues with TS < 4.7

The problem is the subpath import structure you have with janode and trying to match that in the type defs.

For example, you import for JS with:

import Janode from 'janode';
import AudioBridgePlugin from 'janode/plugins/audiobridge';

Having the TS defs mimic the subpath audiobridge plugin import is the hard part. To do so, we need to afaict have the defs in a similar folder structure. However, only TS 4.7 can read package.json in such a way as I have in this pull request to define the subpath imports.

Currently, this PR does it so:

import Janode from 'janode';
import Connection from 'janode/connection';
import Session from 'janode/session';
import AudioBridgePlugin from 'janode/plugins/audiobridge';

If you like, I could clean it up into one .d.ts file and have import on the TS side do something like:

import Janode, {Connection, Session, AudioBridgePlugin } from 'janode';

That would, however, cause conflicts I imagine when you are pulling the type definition for the AudioBridgePlugin from 'janode', but the actual code from 'jandoe/plugins/audiobridge'

Maybe there is a way to do a single .d.ts file and still import from a subpath like janode/plugins/audiobridge, but I couldn't find it. The only way I could find to do the subpath import was the one proposed in this PR that requires the newest TS version.

Thanks for the consideration and happy to follow any suggestions you have.

@augustblack
Copy link
Contributor Author

I fixed a few minor things. I'm sure as I test this out, there will be more.

To test, you will need to add to your project where using janode:

npm install -D typescript@beta.

nightly isn't needed, beta is enough. Fwiw, the TS 4.7 should be released by the end of May.

and then in tsconfig.json for your project, be sure to set:

"moduleResolution": "nodenext",

No hurry on my part for the merge (if at all).

From what I can tell, this PR has no intrusion on the JS side of things and should be safe to merge.
If anything, it might encourage other to contribute.

@rchoffar
Copy link

Thx i was looking for this ! Works well with typescript !

@augustblack
Copy link
Contributor Author

That's great, @rchoffar

I've only been using it on the audiobridge. Please let me know if there are any type issues with the other plugins.

@rchoffar
Copy link

I tried to use it with videoroom plugin and it worked well on 0.x
Unfortunetly i switched to janus 1.x and Janode is not compatible with it.
This is not related to your PR.

@atoppi
Copy link
Member

atoppi commented May 24, 2022

@rchoffar what is not compatible with 1.x ? janode does not still support multistream syntax, but it should work on janus 1.x using the 0.x syntax.
Indeed I regularly use janode with janus 1.x.

If you discovered something that is not working please submit a new issue.

@rchoffar
Copy link

rchoffar commented May 25, 2022

@atoppi No sry that's not what i wanted to say, i wanted to upgrade to 1.x in order to use multistream features but janode does not support multistream syntax as you said.

@augustblack
Copy link
Contributor Author

fwiw, typescript 4.7 has been out for a while now.

making things work is now as simple as: npm install -D typescript

@capital-G
Copy link

Any news on this?

Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

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

just got some time to check the status of this PR

@augustblack I've left initial comments and please also consider the following points:

  1. my current concern is that the intellisense in the Visual Studio Code IDE is not using those types declarations, that makes me think that something in the design/location of files is still not okay.
  2. We should update the defs after the recent releases of janode.

@@ -3,6 +3,8 @@
"description": "Meetecho adapter for the Janus WebRTC Server",
"version": "1.6.2",
"type": "module",
"main": "src/janode.js",
"types": "./types/janode.d",
Copy link
Member

Choose a reason for hiding this comment

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

is this a typo?

},
"files": [
"src/*.js",
"src/utils/*.js",
"src/plugins/*.js"
"src/plugins/*.js",
"types/**/*"
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer specifying subfolders and file extension here (likewise the js)

@@ -0,0 +1,47 @@
import { ServerObjectConf } from './janode.js'
Copy link
Member

Choose a reason for hiding this comment

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

why are we importing js files from d.ts ?

@alexandermaisey
Copy link

Hello, is there any update on this PR?

@atoppi
Copy link
Member

atoppi commented Jun 28, 2024

I just noticed that my comments have never received response @augustblack

@augustblack
Copy link
Contributor Author

I've been updating the ts defs for the audiobridge and streaming plugins incrementally as I am working on my project, but I don't think I'll have the capacity to maintain the type defs for all plugins as janode develops, or even keep things up to date.

I personally think it would be less work and more appropriate to convert janode to TS. #24

Is there any desire to move in that direction? I wish I could help out more.

@atoppi
Copy link
Member

atoppi commented Jul 8, 2024

@augustblack not planned migrating to TS in the short term unfortunately.
I guess we need to find some time to properly support type defs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants