filter callback function doesn't show the JSON parsed message

[lucafaggianelli]

hi, I'm using the filter callback to filter messages, though that callback has a MessageEvent as argument, so I need to JSON.parse the message before applying the filter:

const filter = (msg: MessageEvent) => {
    const { type } = JSON.parse(msg.data)

    return type === topic
  }

then later I use the lastJsonMessage hook property so I guess I parse the message twice... is there another solution to avoid a double parse?

[ewilliams-zoot]

@lucafaggianelli
I too have a pattern that would work really well to use the filtering, but I also don't want to parse twice, so I have to resort to using a string search in the filter function (which is better than parsing, but still not ideal).

Maybe some new options in the options param of useWebSocket? Because I also think it would be beneficial to opt out of JSON parsing if you aren't using JSON. Right now, the hook just catches a parse error and returns an empty object for the lastJsonMessage property.

Maybe there is a way to use an option, useJson?: boolean = true;
that when true, will parse earlier on and send it through both the filter function and lastJsonMessage result.

[lucafaggianelli]

Yeah indeed I would add an argument for those not using json

[ewilliams-zoot]

Here is where the message is being captured in attach-listener.ts (also would need to do the same in attach-shared-listeners.ts):

webSocketInstance.onmessage = (message: WebSocketEventMap['message']) => {
    heartbeatCb?.();
    optionsRef.current.onMessage && optionsRef.current.onMessage(message);
    if (typeof optionsRef.current.filter === 'function' && optionsRef.current.filter(message) !== true) {
      return;
    }
    // ...
    setLastMessage(message);
};

That setLastMessage call triggers the hook to re-run, where it then parses the data into JSON for the lastJsonMessage return value. I think it would be possible to modify this section of code using an option to allow JSON or not, and when it is JSON, passing it into a filter and setting the JSON object directly instead of relying on a hook re-run to parse the raw message data again.

This does sort of break the generally-good rule of calculating values that can be derived from other state, instead of maintaining two pieces of state, but that might be justified by optimization.

[iwollmann]

I am facing the same dilemma now and another thing I noticed is that shouldn't heartbeat take priority over filter in this context?

react-use-websocket/src/lib/attach-listener.ts

Lines 31 to 40 in 94321b8

	if (typeof optionsRef.current.filter === 'function' && optionsRef.current.filter(message) !== true) {
	return;
	}
	if (
	optionsRef.current.heartbeat &&
	typeof optionsRef.current.heartbeat !== "boolean" &&
	optionsRef.current.heartbeat?.returnMessage === message.data
	) {
	return;
	}

Shouldn't this be part of this #259 @robtaussig ?

[ewilliams-zoot]

@iwollmann I would agree with that too.
An unfortunate potential side effect of simply moving the heartbeat check up is that users may accidentally create filters that would return true for heartbeat messages, and then they'd wonder why their filter didn't trigger because that heartbeat check absorbed it.

[iwollmann]

@iwollmann I would agree with that too. An unfortunate potential side effect of simply moving the heartbeat check up is that users may accidentally create filters that would return true for heartbeat messages, and then they'd wonder why their filter didn't trigger because that heartbeat check absorbed it.

Indeed, perhaps a fix for a major release?

[robtaussig]

@ewilliams-zoot @iwollmann - Thank you for your suggestions and I apologize for the very late reply to @ewilliams-zoot 's suggestion last April. I agree that users should be able to opt out of automatic parsing, and just released support for that here in 40a290b

I am reluctant to track lastJsonMessage in another piece of state (as opposed to deriving it), though I appreciate the benefit of parsing the message prior to passing it to the filter option. I toyed with the idea of adding a map function option that would allow you to transform the websocket message to any value you want, which would be passed to filter and ultimately replace the value set in setLastMessage.* I think this could be a promising approach, but it will require a bit of work to make TypeScript happy, and I have limited bandwidth these days. I'd be happy to review/release any PRs if you have an approach you like and some bandwidth yourself!

*As an aside, this would potentially negate the need for the other option to disable automatic json parsing, since we'd likely want to disable that behavior anyway if the user is mapping the websocket messages into potentially something other than a string -- and if the user only wants to disable json parsing, they could simply define map as a function that returns the original websocket message. That said, this would be a bit imperative and I'm not sure this is better than just having both options available.