ryardley / ts-bus

A lightweight JavaScript/TypeScript event bus to help manage your application architecture.
MIT License
135 stars 8 forks source link

Fix missing events during render #33

Closed ryardley closed 5 years ago

ryardley commented 5 years ago

This PR fixes the bug where useBusReducer was loosing initial events that fired between the render and running of useEffect. This issue was described here: https://github.com/ryardley/ts-bus/issues/23

It works by replacing useEffect with useLayoutEffect which runs synchronously during the render process.

codecov[bot] commented 5 years ago

Codecov Report

Merging #33 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #33   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines          56     56           
  Branches       11     11           
=====================================
  Hits           56     56
Impacted Files Coverage Δ
src/useBusReducer.ts 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 98e84a1...53ef794. Read the comment docs.

ryardley commented 5 years ago

This patch also appears to fix the problem with the sandbox:

https://codesandbox.io/s/elastic-heisenberg-occ3b

However if the user uses useLayoutEffect to fire the messages they get lost like before. I am happy with this as it is arguable they probably shouldn't be doing that with the React docs screaming at people telling them to use useEffect and only use useLayoutEffect when they need to specifically.

dmicic commented 5 years ago

@ryardley awesome fix! I came across the useLayoutEffect but somehow, I didn't like the idea of having a dependency on react's lifecycle (which is the case with useEffect as well). To my mind, the bus and the subscribers should be initialized before anything else happens in the app. But this obviously complicates the usage of the library :) Anyway, I like your solution much more than my proposal #23

Just to elaborate why I find this change useful: In my app, I have a central navigation component/logic. Navigation has to be requested via events (using ts-bus). Let's say, somebody enters an invalid/wrong url... in this scenario, I want to redirect a user immediately to a different page. Which means, I must send an event to the navigation component. In this case, the message gets lost given that the useBusReducerhasn't been called yet.

dmicic commented 5 years ago

Tested in my app and it works great! :D