search

re-rxjs / react-rxjs

React bindings for RxJS
https://react-rxjs.org
MIT License
549 stars 19 forks source link

fix(core): ensure that all subscriptions count for the refcount #170

Closed josepot closed 3 years ago

josepot commented 3 years ago

A little bit of context of what this bug is and how it got introduced, since we may need to revisit this in the future:

The issue got introduced by this "fix". Let's first understand what that "fix" was about: If the Subscribe component gets remounted and the only consumers of its source$ are its children, then the desired behavior is that when it remounts the source$ shouldn't have any subscribers. However, that's not what was happening. There was a race-condition in which when the mount logic of the Subscribe component was being executed, the cleanup function for the Subscribe component had been evaluated, but the cleanup function of its children hadn't been evaluated.

In order to "fix" this issue, my a "brilliant" idea was that since we have the guarantee that any time that a "bound hook" gets evaluated, the subscription to its underlying observable must exist already, then there is no need to take the subscription of the those hooks into account for the inner "refcount" of the underlying observable... :man_facepalming: I know, I know...

Anyways, it turns out that the race condition happened because Subscribe was using useLayoutEffect while the "bound hook" uses useEffect, so a better fix was to use useEffect on both and have all subscribers count in the "refcount" of the underlying observable.

codecov[bot] commented 3 years ago

Codecov Report

Merging #170 (9be2eff) into main (b9ee430) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #170   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          375       366    -9     
  Branches        46        44    -2     
=========================================
- Hits           375       366    -9
Impacted Files Coverage Δ
packages/core/src/bind/connectFactoryObservable.ts 100.00% <ø> (ø)
packages/core/src/internal/share-latest.ts 100.00% <ø> (ø)
packages/core/src/Subscribe.tsx 100.00% <100.00%> (ø)
packages/core/src/internal/useObservable.ts 100.00% <100.00%> (ø)

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 b9ee430...d2ac739. Read the comment docs.