Fix error occurring in useEffect on hmr

[PatrykWalach]

Rebase of #264

[rbalicki2]

lgtm, just rewrote the comment slightly for grammar and clarity

[rbalicki2]

Sorry, I forgot to do a thorough review before accepting 😂 Do we also need this in useUpdatableDisposableState?

If so, there are probably enough places where we do this that it makes sense to have a useEffectIgnoringHmrAndStrictMode hook.

It might return the lastCommittedParentCache, in which case we don't need to do anything extra in useCachedResponsivePreCommitValue

[rbalicki2]

    // Since React doesn't know if the code inside of a useEffect has changed,
    // it unmounts and remounts all useEffect calls when doing HMR. This breaks
    // this hook's assumptions! The `onCommit` is called twice.
    //
    // Since this is a library, the user can't change this code, so we are safe
    // to skip this rerun.
    //
    // This also prevents the useEffect from running twice in Strict Mode.

[rbalicki2]

    // Since React doesn't know if the code inside of a useEffect has changed,
    // it unmounts and remounts all useEffect calls when doing HMR. This breaks
    // this hook's assumptions! The cleanupFn is called unexpectedly during HMR,
    // and the item is not recreated. (Even recreating the item would be a bad outcome,
    // as that would reissue network requests.)
    //
    // Since this is a library, the user can't change this code, so we are safe
    // to skip this rerun.
    //
    // This also prevents the useEffect from running twice in Strict Mode.

[rbalicki2]

    // Since React doesn't know if the code inside of a useEffect has changed,
    // it unmounts and remounts all useEffect calls when doing HMR. This breaks
    // this hook's assumptions! The item in the `itemCleanupPairRef` will be disposed.
    //
    // Since this is a library, the user can't change this code, so we are safe
    // to skip this rerun.
    //
    // This also prevents the useEffect from running twice in Strict Mode.

Do we also need to have this check above, in the other useEffect?

[PatrykWalach]

The second one will not throw because it sets the ref to null. However, because of this, we will lose the current state, which is not perfect.

[rbalicki2]

yeah, seems better to not set it to null!

[PatrykWalach]

Oh wait, this should be good, because it will cleanup only when we set with a function and when the ref is still not cleaned up, hmr shouldn't be able to trigger it twice

[rbalicki2]

wdym? Which useEffect are you referring to?

[PatrykWalach]

useEffect(
    function cleanupItemCleanupPairRefAfterSetState() {
      if (stateFromDisposableStateHook !== UNASSIGNED_STATE) {
        if (itemCleanupPairRef.current !== null) {
          itemCleanupPairRef.current[1]();
          itemCleanupPairRef.current = null;
        } else {
          throw new Error(
            'itemCleanupPairRef.current is unexpectedly null. ' +
              'This indicates a bug in react-disposable-state.',
          );
        }
      }
    },
    [stateFromDisposableStateHook],
  );

This one should cleanup only when these two conditions are met, reruning this on hmr has no effect.

[PatrykWalach]

I posted this stuff way too quickly. This will throw on hmr but I think just removing the throw should be enough