[Custom Hook] is my usage of custom hook right? or it's a react bug?

[kkdev163]

//Re-edit by Dan's suggestion at Wed Mar 13 2019 12:38:13 GMT+0800
Do you want to request a feature or report a bug?
I want to report a potential bug, or maybe it's just my code cause this bug?

What is the current behavior?
I followed the official document to create a custom hook demo, the example is here. In this demo, I have 3 references of custom hook [useFriendStatus], but the state is not identical unless I add [] as 2nd params of useEffect(as line 70). wait for 10 seconds, the label(online/offline) was not matched the followed green indicator and picker 's green indicator.

What is the expected behavior?
use or not use [] as 2nd params will not impact the rendering behavior, but only the performance.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

[gaearon]

but the state is not identical

What state is not identical? Not identical to what?

It would help when a bug report contains the exact steps to reproduce, the expected result, and the actual result. Otherwise it's very difficult to understand what you expected the code to do.

[gaearon]

I guess you mean the green indicator doesn't match the "online / offline" label?

[gaearon]

Looking at it, there are two issues in the implementation of the event emitter.

The first one is that subscribe call doesn't immediately fire the callback. This depends on your store implementation (e.g. Rx subscriptions immediately fire it but e.g. Redux doesn't). But if the subscription source changes, it makes sense that you want to query its state immediately.

So you either need to modify the store:

  subscribeToFriendStatus(id, callback) {
    if (!(this.listeners[id] instanceof Array)) {
      this.listeners[id] = [];
    }
    this.listeners[id].push(callback);
+   callback(this.userList[id]);
  },

Or you need to modify the Hook itself to query and set the current state immediately before subscribing.

The second problem (which causes the main issue I'm seeing) is that this event emitter doesn't invoke a listener if it unsubscribes during a dispatch. So what happens is that one component receives a state change and re-renders, causing effects of other ones to re-fire, and unsubscribing their callbacks from the last render just before they were supposed to be called.

The fix to this is to make the event emitter clone the listeners before dispatching:

-   const listeners = this.listeners[id];
+   const listeners = this.listeners[id].slice();
    if (listeners && listeners.length > 0) {
      listeners.forEach((callback, i) => {
        callback(this.userList[id]);
      });
    }

(This is how, for example, Redux solves a similar problem.) You would see the same problem in classes if a change caused by one event would cause subscription source to swap in another component. This is also why [] in the effect prevents this — but it would break the case when id changes over time. So the canonical fix is to clone the listeners, not to specify [] as deps.

With these two fixes, everything appears to work for me: https://codesandbox.io/s/vq73o1yxj7. As a bonus, you can optimize and avoid re-subscribing too often by passing [friendId] as effect deps: https://codesandbox.io/s/42j9xnzop9.

In general, subscribing to event emitters based on mutable data can be a bit gnarly if you want to handle all edge cases. (This is true for classes too.) We'll provide a Hook called useSubscription that handles those well soon so you'll be able to plug it into your project.

Hope this helps! Let me know if you have more questions.

[gaearon]

Here's the version with the useSubscription Hook btw: https://codesandbox.io/s/zn7kq14k9p

function useFriendStatus(friendId) {
  const subscription = useMemo(() => ({
    source: friendId,
    getCurrentValue(id) {
      return ChatAPI.userList[id].isOnline;
    },
    subscribe(id, callback) {
      ChatAPI.subscribeToFriendStatus(id, callback);
      return () => {
        ChatAPI.unsubscribeToFriendStatus(id, callback);
      };
    }
  }), [friendId]);
  return useSubscription(subscription);
}

It handles more edge cases and is generally the recommended way to integrating with event emitters. It does similar things to what React Redux bindings do, for example. (We'll add it to the docs soon.)

[kkdev163]

It's amazing to have your so patient explaining, thanks for a lot. I spend a lot of time to understand this explaining:

The second problem (which causes the main issue I'm seeing) is that this event emitter doesn't invoke a listener if it unsubscribes during a dispatch. So what happens is that one component receives a state change and re-renders, causing effects of other ones to re-fire, and unsubscribing their callbacks from the last render just before they were supposed to be called.

until I write the illustrate demo here, it will console information of the reason for this bug, but I'm not very sure whether my understanding is true.