robtaussig / react-use-websocket

React Hook for WebSocket communication
MIT License
1.63k stars 135 forks source link

The messageHistory in WebSocketDemo has side effects. #98

Closed liuwin7 closed 3 years ago

liuwin7 commented 3 years ago

The example in the README.md, the messageHistory is computed with useMemo.

messageHistory.current = useMemo(() =>
    messageHistory.current.concat(lastMessage),[lastMessage]);

It should be functional and no side effect. However, it not only depends on the other dependence --messageHistory-- expect the dependence list [lastMessage], and also makes a side effect for the messageHistory-- it can override the messageHistory.current.

So, in the strict mode, the messageHistory will add 2 duplicate messages into.

For the business logic, we should use the callback onMessage in the options to add the new message to the history, instead the hooks in React.

useWebSocket(
        'ws://localhost:4000/echo',
        {
            onMessage: event => {
                messageHistory.current = messageHistory.current.concat(event.data);
            },
        }
    );
robtaussig commented 3 years ago

@liuwin7 This is a good point, and I hadn't thought carefully enough when setting up that example -- which is not good since it is reasonable to assume that users will follow/copy README examples for their own usage. I will fix -- thanks!

robtaussig commented 3 years ago

Fixed 9709cf5b626035dc21111520e7c9d409724ec5e2