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

Subscribe support #711

Open
wants to merge 84 commits into
base: master
Choose a base branch
from
Open

Subscribe support #711

wants to merge 84 commits into from

Conversation

ikq
Copy link

@ikq ikq commented May 20, 2021

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

jmillan commented May 21, 2021

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

jmillan commented May 25, 2021

@ikq

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

@ikq
Copy link
Author

ikq commented Mar 1, 2023

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

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

@devoxy1
Copy link

devoxy1 commented Oct 27, 2023

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

@jmillan
Copy link
Member

jmillan commented Nov 1, 2023

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

ikq commented Nov 2, 2023 via email

@jmillan
Copy link
Member

jmillan commented Nov 2, 2023

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

jmillan commented Nov 2, 2023

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.

lib/Notifier.js Outdated Show resolved Hide resolved
@jmillan
Copy link
Member

jmillan commented Nov 2, 2023

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

jmillan commented Nov 2, 2023

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

jmillan commented Nov 3, 2023

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

devoxy1 commented Nov 15, 2023

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

@devoxy1
Copy link

devoxy1 commented Nov 28, 2023

Kind reminder

@devoxy1
Copy link

devoxy1 commented Dec 5, 2023

Up!

@ddyner
Copy link

ddyner commented Dec 10, 2023

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

@jmillan
Copy link
Member

jmillan commented Dec 11, 2023

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

ikq commented Dec 11, 2023

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

ikq commented Dec 17, 2023

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

@devoxy1
Copy link

devoxy1 commented Jan 9, 2024

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

@jmillan
Copy link
Member

jmillan commented Jan 9, 2024

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

@jmillan
Copy link
Member

jmillan commented Jan 9, 2024

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

devoxy1 commented Jan 9, 2024

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

@jmillan
Copy link
Member

jmillan commented Jan 18, 2024

I'm not forgetting about this...,

@devoxy1
Copy link

devoxy1 commented Mar 28, 2024

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

@orgads
Copy link

orgads commented Jul 23, 2024

@jmillan Ping?

@jmillan
Copy link
Member

jmillan commented Jul 29, 2024

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

Looking forward with this merge also 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.