fix(useThrottleCallback): Cleared timeout on unmount

[paul-sachs]

What is the current behavior, and the steps to reproduce the issue?

• NextJS application
• Use useThrottledCallback anywhere
• Notice that it's only called once and then never again

What is the expected behavior?

Method should be called and limited to throttling frequency.

How does this PR fix the problem?

Clears the timeout properly.

Checklist

• Have you read contribution guideline?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Is there an existing issue for this PR?
• Have the files been linted and formatted?
• Have the docs been updated to match the changes in the PR?
• Have the tests been updated to match the changes in the PR?
• Have you run the tests locally to confirm they pass?

[paul-sachs]

Not 100% sure of what the source of the problem is. It feels like it should be fine more multiple unmounts to be called, since if it unmounts, the ref should be reinitialized. Tried disabling strict more but it didn't change the behaviour.

Given that this same logic is already being done in useDebounceCallback, figured its pretty safe.

[xobotyi]

could you please, at least attempt, to cover the issue with tests - if not - make an issue with thorough WTR

[ArttuOll]

I'll second what xobotyi is saying. Could you please give us a CodeSandbox that demonstrates this bug? Or a failing test? Given those, we can make sure that this bug is fixed.

[paul-sachs]

Yeah that's fair. I'll see what I can do to reproduce. A sandbox with nextjs isn't reproducing so maybe it really is something i'm doing.

[paul-sachs]

Turns out it was strict mode in next js that was causing it to fail. At least, in my case, when i disabled reactStrictMode in next config, it's fixed. I still can't seem to get a sandbox to reproduce though, so it might be a combination of reactStrictMode and something else.

[paul-sachs]

I think I might have just been asleep at the wheel and accidentally using useDebounceEffect instead of useThrottledEffect. I reproduced the problem with this sandbox here. I'm trying to see if i can reproduce it without nextjs, but it definitely is tied to strict mode. Seeing if I can write tests for this.

[paul-sachs]

Hmm can't seem to reproduce the problem in unit tests:

const { rerender } = renderHook(
  (dep) => {
      useThrottledEffect(spy, [dep], 200, true);
  },
  {
    initialProps: 1,
      wrapper: StrictMode as ComponentType<number>,
  }
);

This may be due to a subtle difference between actual browser engine and nodejs. I can push the test but it's not actually detecting the bug seen the browser, so probably not valuable.

[ArttuOll]

Your change indeed fixes the problem. Thank you. I'll be looking more into this later. For now, let's run the workflows.

I'm not certain why this change is necessary though. I'm pretty sure it has something to do with the same timeoutID getting stuck into the ref if it's not cleared on unmount, but I can't quite grasp the precise mechanics of this yet.

It feels like it should be fine more multiple unmounts to be called, since if it unmounts, the ref should be reinitialized.

useRef does store the information between renders and returns the same object every time.

[codecov]

Codecov Report

Merging #1173 (175758d) into master (284e499) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1173   +/-   ##
=======================================
  Coverage   98.49%   98.49%           
=======================================
  Files          62       62           
  Lines        1064     1065    +1     
  Branches      179      179           
=======================================
+ Hits         1048     1049    +1     
  Misses          2        2           
  Partials       14       14           

Impacted Files	Coverage Δ
src/useThrottledCallback/index.ts	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ArttuOll]

It seems to be the case that if we don't clear the timeout.current on unmount, the same timeoutID gets stuck in it, which causes the hook to hit the if-statement in the wrapped function every time, leading to the execute function only being run once. I could not see StrictMode having any effect on this behavior.

I would be interested to hear if @xobotyi has any insight into this.

I also had trouble writing a test for this behavior. I tried removing the clearing of the timeoutID from useDebouncedCallback as well and seeing if any of its tests would fail, but none did. However, this change breaks none of the existing tests and demonstrateably fixes the bug in the linked CodeSandbox, so I'm approving this.

[paul-sachs]

@ArttuOll thanks for looking into it, it does seem a little weird that we can't reproduce it within tests 🤷 . Let me know if there's anything else I can do to move this on. I'm currently using a vendored version of this function just so it doesn't cause confusion with my team due to this behaviour. Would love to update.

[ArttuOll]

@ArttuOll thanks for looking into it, it does seem a little weird that we can't reproduce it within tests 🤷 . Let me know if there's anything else I can do to move this on. I'm currently using a vendored version of this function just so it doesn't cause confusion with my team due to this behaviour. Would love to update.

At this point, it's only a matter of @xobotyi approving and merging this.

[xobotyi]

Welp this one is interesting =)

Take a look at this sandbox.

• fix adds actons only to the component unmount stage - not the intermediate, a complete one, therefore i really wonder why this fix works.
• the strict mode engages all effects to be triggered twice - therefore it triggers throttle immediately, so the following sequence is performed:
  1. first effect invoked - it attempts to update the state, and succeds, state not applied since it is only run caused by strict mode
  2. second effect invoked immediately - throttle kicks in and discards the call and it's resulting state is applied (no state change)

As far as i can imagine the fix works due to internals of strict mode which (it is only a guess) is calling final unmount for the shadowed state and therefore clearing the throttle timeout for second call effect.

That assumtion needs deep further investigation.
I understand that we're leaving (maybe) broken code live - but id better spend more time (that i sadly have quite a fiew atm) investigating rather than taking a shot in the sky

[xobotyi]

🎉 This PR is included in version 23.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀