Subscribe support

[ikq]

I added Subscriber.js Notifier.js to JsSIP

I tried write them according JsSIP style and lint errors.
Please take a look and let me know what needs to be fixed.

I already see that *.d.ts files are missing.

The work still in progress: I want add to SUBSCRIBE Contact +sip.instance
as you do in REGISTER

[jmillan]

Hi @ikq,

JsSIP code must have the same style in order to make it readable and maintainable. Comments before making a deeper review:

• Debug line as the first method call in al public method implementations.
• New line after return.
• New line before and after call to super().
• First conditional in a block starts in new line and last ends in a new line.
• Conditional blocks always contain curly brackets.
• Comments in the line above when possible.
• Calls to debug have usually a new line above and a new line below.

Please take a certain file (ie: RTCSession.js) as a reference and try to stick to its style.

[jmillan]

@ikq

Thanks for the cleanup, 👍 We'll review the PR as we can.

[ikq]

Apparently, this is a little needed feature. ;-)

Nevertheless, the code is shared, whoever needs it can use it.

[devoxy1]

Hello! This is really good feature and we would like to use it. Is there chance this can be merged?

[jmillan]

@ikq, I've re-reviewed the code and it's in good shape. The only missing part is the documentation. I'll prepare a new release tag so you can add them for the upcoming release.

[ikq]

Hi Jose Luis Millan, What's wrong with the documentation? How can it be improved? I thought you automatically generated it from comments. It seems that the subscriber.js/notififer.js functions have such comments. UA.js /** * Create subscriber instance */ subscribe(target, eventName, accept, options) { logger.debug('subscribe()'); return new Subscriber(this, target, eventName, accept, options); } /** * Create notifier instance */ notify(subscribe, contentType, options) { logger.debug('notify()'); return new Notifier(this, subscribe, contentType, options); } Notifier.js /** * @param {UA} ua - JsSIP User Agent instance. * @param {IncomingRequest} subscribe - Subscribe request. * @param {string} contentType - Content-Type header value. * @param {NotifierOptions} options - Optional parameters. * @param {Array<string>} extraHeaders - Additional SIP headers. * @param {string} allowEvents - Allow-Events header value. * @param {boolean} pending - Set initial dialog state as "pending". */ constructor(ua, subscribe, contentType, { extraHeaders, allowEvents, pending }) ... Exists also test as usage example: test-UA-subscriber-notifier.js and my test: https://github.com/ikq/subscribe_notify_test/blob/main/test.js Тhe tests are useful, because I myself have forgotten how to use this API.

[jmillan]

What's wrong with the documentation?
I thought you automatically generated it from comments.

Unfortunately not. We use a static website for the doc, and it's not automatically generated.

https://github.com/versatica/jssip.net

This has more than few years.., since JsSIP come up, and hence we are a bit outdated there.

I'll write a little README.md there with instructions on how to build and modify the docs.

[jmillan]

A little README added to the JsSIP web repo. I've also created the 3.11.x version template branch here.

The files to be modified are those under this folder: content/documentation/3.11.x/

You will need to, at least, modify api/ua.html and create api.subscriber.html and api.notifier.html ones.

I know we should have autogenerated docs.., we would like to have the resources to do so.

Let us know if you find any issues.

[jmillan]

Тhe tests are useful, because I myself have forgotten how to use this API.

I know, and I very much appreciate it but we need to have the public API documented in the website. I know by experience it's a hassle.., we'll try to move to autogenerated docs as we can.

[jmillan]

@ikq, please, do not rely on the documentation branch I did for 3.11 version. I'm going to remove it right now.

We are going to have documented just the latest version, so after this PR is merged, just clone it and add the docs there.

We definitely need to have the docs autogenerated from the inline comments. We already do it in rtp.js.

[jmillan]

@ikq, please take merge master branch into this. I've added a new eslint rule no-trailing-spaces. There are many trailing spaces in this PR.

[devoxy1]

@jmillan Would it be possible to review again? Thanks!

[devoxy1]

Kind reminder

[devoxy1]

Up!

[ddyner]

Hello. We also want to use this feature. Will this pull request be merged?

[jmillan]

This is waiting for the docs to be generated. @ikq are you willing to create a PR for the doc? We cannot merge the feature without it.

[ikq]

Writing documentation is never easy for me but let's try.

How do you do this?

I have to download the zip (or git clone) from https://github.com/versatica/jssip.net,

install nanoc and modify content/documentation/api/ua.html

And then check the result using nanoc compile, nanoc view?

If the modified doc compiled, make Pull Request of https://github.com/versatica/jssip.net with

modified api/ua.html,
and created api.subscriber.html, api.notifier.html

Do I understand correctly ?

[ikq]

I created pull request for https://github.com/versatica/jssip.net
Please take a look.

[devoxy1]

@jmillan Hello! Could you please check it up? Thank you!

[jmillan]

I'll review when I can. If you are in a hurry, please do use this branch freely.

[jmillan]

I'll review when I can. If you are in a hurry, please do use this branch freely.

This way you will also collaborate on using, testing and reporting.

[devoxy1]

@jmillan We already using it this way :) but it's a bit difficult to maintain separate branch.
Anyways, thanks you!

[jmillan]

I'm not forgetting about this...,

[devoxy1]

@jmillan Hello! Any chance you had time to review this one?
Thanks!

[orgads]

@jmillan Ping?

[jmillan]

For those willing to use this feature I would suggest you use this branch. I don't have at the moment the resources to re-check and merge this, so please, do use this branch for the time being.

[johnmarkovalo]

Looking forward with this merge also 👍