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

Tolerate already extracted data coming from on message handler from underlying websocket #15

Closed
gpolakow opened this issue Nov 29, 2018 · 7 comments

Comments

@gpolakow
Copy link

Hi there,
I have to use ws package because it supports setting custom headers when opening a websocket, but it turns out that it doesn't cooperate well with the websocket-as-promised. ws implements all the needed methods but in the on('message', ... handler it doesn't pass the full event, but it passes the already extracted data property. Would you mind adding a little check of the incoming parameter? It's just a matter of changing

  _handleMessage(event) {
    const message = event.data;

to

  _handleMessage(event) {
    const message = typeof event === 'string' ? event : event.data;

in the index.js. It stays compatible with the already supported websocket implementations and, at the same time, tolerates the use of ws. I will be happy to send PR for this if you're willing to accept such a hack.

@vitalets
Copy link
Owner

vitalets commented Dec 3, 2018

Hi @gpolakow !
onmessage must receive event parameter by spec.
Could you show your code, maybe we will find out how to achieve it?

@gpolakow
Copy link
Author

gpolakow commented Dec 3, 2018

Hi Witalij!
Well, the ws is not my code, I'm just trying to use it together with the websocket-as-promised. The pecularity of the ws is that it behaves differently when assigning a callback by .on('message',... and .onmessage = .... The first one returns extracted data, and the second one returns whole the event. Check this out:

const WebSocket = require('ws');
const ws = new WebSocket('wss://echo.websocket.org');

ws.on('open', () => {
  ws.send('something');
});

ws.on('message', data => 
  console.log('ws.on(\'message\', ...):', data)
);

ws.onmessage = event =>
  console.log('ws.onMessage: ', event.data);

The result is:

ws.on('message', ...): something
ws.onmessage:  something

Technically, the spec you cited addresses the .onmessage method only, and ws complies with it. The .on() method is apparently not standardized, and that is the one websocket-as-promised uses.

@vitalets
Copy link
Owner

vitalets commented Dec 3, 2018

The .on() method is apparently not standardized, and that is the one websocket-as-promised uses

Agree.
Could you show how you use ws with websocket-as-promised?
It seems to me that we can just redefine _handleMessage of WSP instance.

@gpolakow
Copy link
Author

gpolakow commented Dec 4, 2018

Could you show how you use ws with websocket-as-promised?

After I create wsp instance with something like (note the use of custom headers, that is why I insist on the use of ws):

wsp = new WebSocketAsPromised('ws:example.com', {
  createWebSocket: url => new WebSocket(url, {
    rejectUnauthorized: false,
    headers
  }),
});

I overwrite the _handleMessage method with:

//fix for the websocket-as-promised to work with the ws
wsp._handleMessage = event => {
  const message = typeof event === 'string' ? event : event.data;
  wsp._onMessage.dispatchAsync(message);
  wsp._handleUnpackedMessage(message);
};

where the only change is the check for the type of value returned by the .on() handler. If the type is string it means that the data is already extracted, otherwise it is as it where before - the data is being extracted from the event.

@vitalets
Copy link
Owner

vitalets commented Dec 7, 2018

I've got your point.
It is possible to access original ws object via wsp.ws and subscribe to message event:

wsp.ws.on('message', data => ...)

Doesn't it solve in your case?

@vitalets
Copy link
Owner

@gpolakow
Working with ws package is now officially supported (since 0.10.0).
You should define extractMessageData option that customize how data are extracted from event.
In case of ws data is event itself, so they are proxied as is:

const WebSocketAsPromised = require('websocket-as-promised');
const WebSocket = require('ws');

const wsp = new WebSocketAsPromised('ws://example.com', {
  createWebSocket: url => new WebSocket(url),
  extractMessageData: event => event, // <- this is important!
});

@gpolakow
Copy link
Author

That's cool, thx!

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

No branches or pull requests

2 participants