search

callthemonline / react-sip

React wrapper for jssip
https://www.npmjs.com/package/react-sip
MIT License
50 stars 46 forks source link

Add pathname prop #18

Closed Steveb-p closed 6 years ago

Steveb-p commented 6 years ago

Add websocket suffix option due to some servers (asterisk in particular) usually exposing their wss under specific "directory"

kachkaev commented 6 years ago

Hi @Steveb-p, thanks for this PR! Could you please describe this new option in README and also fix the failing CI?

@denisnikulin could you please review this afterwards?

Steveb-p commented 6 years ago

@kachkaev Naturally, I'm in the process of integrating sip-react into my own app and will do the update on this PR once I'm finished ;)

Steveb-p commented 6 years ago

@kachkaev This should do. Shall I squash commits for you?

denisnikulin commented 6 years ago

good job @Steveb-p, we didn't test it with suffix is the path although it can well be the case for more convenient http server use. the changes look good to me @kachkaev, happy to release once checks pass.

Steveb-p commented 6 years ago

Requested changes integrated ;)

kachkaev commented 6 years ago

Sorry @Steveb-p I found another small concern, which I'd like to discuss. Sorry for being so scrupulous, just want to avoid any breaking changes in future 😅

After looking at how some URI parsers work (e.g. url or url-parse, I noticed that they are using term pathname rather than path and start it with a slash. WDYT of using the same name and format? It might be a bit easier for some future users to set this new prop given that the value can come out of a parser. Are there any cons in this change except that it will mean another commit for you? 😃

Steveb-p commented 6 years ago

@kachkaev I'm ok with it ;) I was wondering about format myself, and as for name the more conventional and understandable it is, the better.

This also allows for the wssUrl variable construction to go back to a one liner.

Steveb-p commented 6 years ago

@kachkaev Integrated name and format changes. Websocket URI construction is back as a one-liner.

Steveb-p commented 6 years ago

@kachkaev Introduced the default value change and squashed commits, since it started to bloat the history ;)

Steveb-p commented 6 years ago

@kachkaev I just like to confirm that with these changes react-sip works with my dev Asterisk with it's default settings, that is to put websockets under /ws. Thanks for fast feedback ;)

denisnikulin commented 6 years ago

@Steveb-p we have released the new version 0.7.0 with your change: https://www.npmjs.com/package/react-sip

thanks!

kachkaev commented 6 years ago

Thanks for your work @Steveb-p! Enjoy 0.7.0 🎉