Run subscriber immediately

[dmicic]

The subscriber function should run immediately when the useBusReducer method gets called, otherwise it could lead to lost events.
useEffect gets triggered only after component has been rendered.

ts-bus/src/useBusReducer.ts

Line 30 in 23de6fb

useEffect(() => subscriber(dispatch, bus), [subscriber, dispatch, bus]);

Example:

<App>
  <BusProvider ...>
    <!-- The MyContextProvider calls useBusReducer(). The reducer subscription gets executed when the component completed rendering. -->
    <MyContextProvider>
      <!-- Component publishes a message. It does not matter whether this happens during or after render. This component here completes rendering before MyContextProvider, and therefore, the message gets published before reducer subscription has been set up.    -->
      <MyComponent />
    </MyContextProvider>
  </BusProvider ...>
</App>

The useBusReducer should call the subscriber method immediately and only once.
Pseudo-code solution:

export function useBusReducer<E extends BusEvent = BusEvent, T = any>(
  initState: T,
  reducer: (s: T, a: E) => T,
  subscriber: (
    dispatch: DispatchFn<E>,
    bus: EventBus
  ) => void = defaultSubscriber
) {
  // Pull the bus from context
  const bus = useBus();

  // Run the reducer
  const [state, dispatch] = useReducer(reducer, initState);

  const subscriberCallback = useCallback(() => subscriber(dispatch, bus), [subscriber, dispatch, bus]);
  const [subscriber, setSubscriber] = useState();

  if (subscriber !== subscriberCallback) {
    subscriberCallback();
    setSubscriber(() => subscriberCallback);  
  }

  return state;
}

I am not aware of a more elegant solution without the useState hook... :/

Running code like the creation of subscriptions during the render phase may not follow React's best practises, however, it would still make life easier in some cases and it's up to the developer not to missuse it :)

[ryardley]

You are right we get a render before the busReducer is subscribed to the event bus but everything I have read about React hooks suggests to do this.

We should not run sideeffects in render which is what calling subscriberCallback() is going to do. It stops the component being functional. You could make the case that if idempotent it is ok but React really isn't designed to work like that and does lots of weird memoization and caching as it assumes each render is pure and I am not sure I want to track exactly what react is doing. The render function might actually be run twice with subscriber state being undefined. There is no guarantee it is only run once. The is the point of useEffect it that it is a guarantee something is run once separate from the render stage.

I did notice a bug now you bring this up. The following:

function defaultSubscriber<E extends BusEvent>(
  dispatch: DispatchFn<E>,
  bus: EventBus
) {
  bus.subscribe<E>("**", dispatch);
}

Should probably be:

function defaultSubscriber<E extends BusEvent>(
  dispatch: DispatchFn<E>,
  bus: EventBus
) {
  return bus.subscribe<E>("**", dispatch);
}

The idea being that:

  // Run the subscriber
  useEffect(() => subscriber(dispatch, bus), [subscriber, dispatch, bus]);

Will actually unsubscribe every time the dependencies change.

Perhaps I will write a test for this later this week and fix it.

[ryardley]

@dmicic Can you share a repo demonstrating the missing events? Maybe there are other ways to gather missing events and ensure they are replayed correctly. In my head I am thinking of passing a special bus to collect events and then playing them back once subscribed or pausing events and queueing events until the bus is subscribed

[dmicic]

@ryardley Please have a look at this code here: https://codesandbox.io/s/focused-swirles-2hbms

TLTR: Yes, I fully agree with your comments and I believe some kind of queuing mechanism would be good enough to solve my issue and at same time, still be 'hooks compliant'.

You can look at this problem from different perspective. The first one, and probably the most important one, is what you describe in regards to the correct implementation with react hooks.
The other perspective is from a solution architecture point of view, where you want to achieve decoupling of your components using a bus like ts-bus. As it stands now, I would have to introduce some extra code outside my component in order to do what the reducer is supposed to do (speaking about the app in which I encountered the issue). That means:

• I spread the logic of my component as I have to move some logic closer to the creation of the of the state. Which is not an ideal solution as it does not belong there.
• My system becomes more tighly coupled. That's basically the consequence of bullet point 1.

[dmicic]

@ryardley Any thoughts on this issue? I think a queuing solution could be prone to error. How long does the events need to be queued? How does the bus now whether events have to be pushed to newly subscribed reducer? (maybe the events happened in the passed, not during the actual render stage. In that case, new subscribes must not receive "old" events). Hence, bus must now when to start and stop recording the events.

Maybe there is a simpler solution to that problem by changing the behavior in the bus. The subscribe method of the eventbus should initially remove the handler (if it exists) and then (re-)subscribe again.
The handler method should be memoized using useCallback for instance, otherwise the code will not work properly and we would end-up with double subscription.

    const type = getEventType(eventType);
    // Remove first if it exists already.
    this.emitter.removeListener(type, handler);
    this.emitter.on(type, handler);
    return () => this.emitter.off(type, handler);

The useBusReducer could be changed to:

  const unsub = subscriber(dispatch, bus);
  useEffect(() => unsub, [subscriber, dispatch, bus]);

Assuming the dispatch is memoized in this situation, the code would not create multiple subscriptions, even if it function gets executed multiple times.

Doesn't look like a perfect solution... But so far, all tests are still green :)

[ryardley]

I am a little fuzzy as to what you mean here regarding subscription/unsubscription and how that helps. I am also travelling and away from a computer at the moment so I imagine once I play around with a potential fix this might make sense to me. What I meant to do that I haven’t had a chance to here would be to create a PR with a test case based on your example code that tests for the error and demonstrates the failing case quickly followed by implementing a fix. I have felt this an edge case that requires a complex fix so it is a little lower on the priority list in my head but if it’s urgent for you then let’s try and get this done?

[ryardley]

Ok I have a failing test case here: https://github.com/ryardley/ts-bus/tree/fix-missing-events-during-render if I get time this afternoon I might be able to look at the solutions you suggest here.

[ryardley]

So the fix appears simple which is to change useEffect to useLayoutEffect. See the following PR: #33

[ryardley]

This is now live