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

Create client with custom validateMessage and parseMessage #409

Closed
sklymoshenko opened this issue Oct 5, 2022 · 8 comments
Closed

Create client with custom validateMessage and parseMessage #409

sklymoshenko opened this issue Oct 5, 2022 · 8 comments
Assignees
Labels
question Further information about the library is requested

Comments

@sklymoshenko
Copy link

Story

Hi, thank you for doing all work by creating and managing this repo its great! Maybe my usecase will be not wide used but anyway.

I have a client that has a proxy server and then regular server. When i establish a ws connection, by proxy server i create a websocket stream by createWebSocketStream() from ws package and basically communicating with binary data.
As i can see graphql-ws has a restriction thats in validateMessage checks if message that comes from ws is a JSON or UTF8 as you wish.

I just want to be able to obtain this information that this is a buffer or another type and translate it to a UTF8 or JSON so graphql-ws wont fail on this validation error.

It happens at the beginning when I send { type: "connection_init" } my server gets it, and returns { type: "connection_ack"} but proxy server makes it a binary stream and on client i get Binary Message instead of JSON. If i copy this resposne as UTF8 i can see that this is a correct { type: "connection_ack"} but it comes as a Blob and fails on Message is missing the 'type' property inside validateMessage from graphql-ws because its not JSON and doesnt have a type property.

I must use proxy with binary data for another technical reason that isnt valuable, so its not an option to just simply swith to JSON messages for ws.

Simply trying converting message from ws before sending it to a validateMessage or parseMessage function is enough. Or can it be exposed to a createClient() options function so it will be configurable for other people too ?

Something like

export { createClient } from "graphql-ws"

const client = createClient({
  url: "ws://localhost:3001/graphql,
  parseMessage: async (message) => if (message instanceof Blob) return  await message.text()
})

Its just need to be called before a validateMessage().

Or in a current validateMessage just check for Blob and parse it before do actual validation.

const message = message instanceof Blob ? await data.text() : data
return validateMessage(
    typeof data === 'string' ? JSON.parse(data, reviver) : data,
  );

Regardless of mine reasons not to use a JSON over websockets messages people use binary streams as its less data and less weight, can be transported bigger chunks and means more users and so on.

Thank you!

@enisdenjo
Copy link
Owner

enisdenjo commented Oct 7, 2022

Hey there,

JSON must be used because that's the requirement as per the protocol. There's also a statement:

Messages are represented through the JSON structure and are stringified before being sent over the network.

GraphQL over WebSocket @ Communication

I feel like what you need can be achieved by augmenting the WebSocket implementation on the client, roughly:

import { createClient, CloseCode } from 'graphql-ws';

class MyWebSocket extends WebSocket {
  constructor(address: string, protocols: string | string[]) {
    super(address, protocols);

    super.onmessage = (e) => {
      (async () => {
        let data: string;
        if (e.data instanceof Blob) {
          data = await e.data.text();
        } else if (typeof e.data === 'string') {
          data = e.data;
        } else {
          throw new Error(`Unsupported message data type ${typeof e.data}`);
        }

        this.onmessage(new MessageEvent(e.type, { data }));
      })().catch((err) => {
        // make sure to catch errors, report them to graphql-ws and close the socket
        this.onerror?.(err);
        this.close(CloseCode.InternalClientError);
      });
    };
  }

  public onmessage = (_e: MessageEvent<string>) => {
    // will be replaced by graphql-ws
  };
}

const client = createClient({
  webSocketImpl: MyWebSocket,
  url: 'ws://localhost:3001/graphql',
})

I would greatly appreciate if you give the above example a try. If it doesn't work out, please write back and I'll think about the alternatives. Thanks!

@sklymoshenko
Copy link
Author

Hi, thank you for response.
I did something similar and implemented my own version for handling this kind of situation.
I am just curiose if WebSocket interface has an option to make it messages as binary WebSocket.binaryType why grapqhql-ws protocol is sticking just to JSON ? As, as i mentioned above, it will reduce a data weight for messages that will lead to improved performance. And to fix it, or to let it use binary data, from my surface observation is just to turn Buffer into JSON before validation and parsing it ?

I am sure that it might be much more complicated, or creates different types of problems, that why i am asking.

Thank you for response in advance.

@enisdenjo
Copy link
Owner

I did something similar and implemented my own version for handling this kind of situation.

So you got it working? Glad to hear that.

why grapqhql-ws protocol is sticking just to JSON ?

Because the messages have to be of some format, the format used everywhere throughout GraphQL is JSON, so I've just stick with that in favour of familiarity and ease of usage.

Do note that the protocol is still an RFC, and therefore we could think about elevating the "stringify" requirement in the protocol - but the format of the message itself should stay as JSON. What do you think? PRs are very welcome. 😄

@sklymoshenko
Copy link
Author

Yeah, i agree that the format should remain JSON, it makes sense. I will create a PR for elevating requirements for stringify method.
It makes sense for me to support both data representation that WebSocket API is providing, so it means this elevations of stringify mehtod shouldnt be hundrends, bc we can have either JSON or Binary and it wouldnt polute codebase with more and more elevations.

@enisdenjo
Copy link
Owner

Cool, looking forward to the PR.

Closing this issue in favor of the solution at: #409 (comment).

@enisdenjo enisdenjo added the question Further information about the library is requested label Oct 7, 2022
@enisdenjo enisdenjo self-assigned this Oct 7, 2022
@esseswann
Copy link

esseswann commented Jun 16, 2023

Hey there,

JSON must be used because that's the requirement as per the protocol. There's also a statement:

Messages are represented through the JSON structure and are stringified before being sent over the network.

GraphQL over WebSocket @ Communication

I feel like what you need can be achieved by augmenting the WebSocket implementation on the client, roughly:

import { createClient, CloseCode } from 'graphql-ws';

class MyWebSocket extends WebSocket {
  constructor(address: string, protocols: string | string[]) {
    super(address, protocols);

    super.onmessage = (e) => {
      (async () => {
        let data: string;
        if (e.data instanceof Blob) {
          data = await e.data.text();
        } else if (typeof e.data === 'string') {
          data = e.data;
        } else {
          throw new Error(`Unsupported message data type ${typeof e.data}`);
        }

        this.onmessage(new MessageEvent(e.type, { data }));
      })().catch((err) => {
        // make sure to catch errors, report them to graphql-ws and close the socket
        this.onerror?.(err);
        this.close(CloseCode.InternalClientError);
      });
    };
  }

  public onmessage = (_e: MessageEvent<string>) => {
    // will be replaced by graphql-ws
  };
}

const client = createClient({
  webSocketImpl: MyWebSocket,
  url: 'ws://localhost:3001/graphql',
})

I would greatly appreciate if you give the above example a try. If it doesn't work out, please write back and I'll think about the alternatives. Thanks!

I don't think it works because onmessage is being replaced in both super and this so there is no way to override it if somebody wants to use MessagePack for example

@enisdenjo
Copy link
Owner

I don't think it works because onmessage is being replaced in both super and this so there is no way to override it if somebody wants to use MessagePack for example

Are you sure? Have you tried it? I remember doing some tests and it worked as expected.

@esseswann
Copy link

esseswann commented Jun 19, 2023

@enisdenjo

Yes, I just tried it and eventually had to make a complete rewrite of WebSocket with only one onemessage method being different. Maybe this is create-react-app babel is messing up the code, I'm not sure

Code I wrote is something along thes lines, it's not very robust and has bad typing, but it's just to check the hypothesis:

class WebSocketImpl {
  private base: WebSocket
  public readyState: number = WebSocket.OPEN
  public onopen: any
  public onclose: any
  public onerror: any
  public send: any
  public close: any
  public onmessage: any

  static CLOSING = WebSocket.CLOSING
  static CLOSED = WebSocket.CLOSED
  static OPEN = WebSocket.OPEN
  static CONNECTING = WebSocket.CONNECTING

  constructor(url: string, protocol: string | string[]) {
    this.base = new WebSocket(url, protocol)
    this.base.onopen = (e) => this.onopen?.(e)?.then?.()
    this.base.onclose = (e) => this.onclose?.(e)?.then?.()
    this.base.onerror = (e) => this.onerror?.(e)?.then?.()
    this.base.onmessage = async (e) => {
      // FIXME needs more checks
      this.onmessage({ data: msgpack.parse(data) })
    }
    this.send = (e: any) => this.base.send(e)
    this.close = (e: any) => this.base.close(e)
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information about the library is requested
Projects
None yet
Development

No branches or pull requests

3 participants