noopkat / ms-bing-speech-service

NodeJS service wrapper for Microsoft Speech API and Custom Speech Service
MIT License
82 stars 17 forks source link

remove headerParams in Websocket #27

Closed DemianD closed 5 years ago

DemianD commented 6 years ago

In Chrome, I get an error:

image

This is caused by passing null parameters when creating a WebSocket.

But these parameters are necessary for the WebSocket in the Node.JS implementation. I'm not sure how I can fix this

Looking forward for feedback :)

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 76.647% when pulling 7454da93af2ef9206dbebd12339e9928338c59df on DemianD:chrome-issues into dc2db6d3e4f7dab5ef5860707f5ad9920fe7e45d on noopkat:master.

noopkat commented 6 years ago

Hi @DemianD !

Thanks for opening this PR. It does appear to resolve the issue for Bing Speech Service use, but unfortunately it breaks usage with Custom Speech Service, because it needs the authorisation headers when this library is run within NodeJS environments.

We'll need some additional logic in order to selectively include these arguments depending on which speech API endpoint you're using. Alternatively, we can also filter by whether or not we are running this in a browser environment.

Thoughts?

Thanks again 🙏

DemianD commented 6 years ago

Hi @noopkat

I've updated the PR. Now, it will determine based on the BABEL_ENV which versions needs to be executed (with parameters, or without).

As an extra, I've made sure that the death part is eliminated from the build.

Thanks for reviewing! 😊

DemianD commented 6 years ago

I'm facing another issue that there is no way to pass an Authorization Header using https://developer.mozilla.org/en-US/docs/Web/API/WebSocket to pass an access_token.

Maybe using a package like https://github.com/theturtle32/WebSocket-Node can help (it has browser support), and there is an option to pass headers: https://github.com/theturtle32/WebSocket-Node/blob/master/docs/WebSocketClient.md#methods

This library is also used in the Javascript SDK of IBM Watson Speech To Text: https://github.com/watson-developer-cloud/speech-javascript-sdk/blob/master/speech-to-text/recognize-stream.js#L22

noopkat commented 6 years ago

@DemianD thanks for looking into this on a deeper level!

One catch is that we're already using WebSocket-Node, but as covered in the docs we can't use the auth headers which is super annoying:

When running in a browser (for example by using browserify) the browser's native WebSocket implementation is used, and thus just the first and second arguments (requestUrl and requestedProtocols) are used (those allowed by the W3C WebSocket API).

So we still need to support the query params for the browser, but also be able to utilise the auth headers in the use case of Custom Speech Service running within NodeJS.

What I'd like to implement / see implemented is a smarter and more encapsulated way of judging at runtime if custom speech service is being used by the developer, without asking them for any further properties or requiring any API changes. This way, we can have a more robust way of being able to switch to using those auth headers in that specific case.

Thanks for sticking around with this pull request - I feel it's getting super close and I love the work so far ✨

noopkat commented 5 years ago

Hi @DemianD,

Thanks for this contribution, and apologies that it didn't get merged in the end. Since then, there's been a new official version of a NodeJS supported SDK for the unified Microsoft Speech Services (formally Bing Speech Service). I'd recommend checking that out instead, if you still have need for a library such as this: https://github.com/Azure-Samples/cognitive-services-speech-sdk

Therefore I'll respectfully close this pull request and will be deprecating / archive this repo 🙇‍♀ Thanks again for your contribution.