toss / suspensive

Manage asynchronous operations, timing, error handling, detecting intersection of elements, and caching easily (One of TanStack Query community resources)
https://suspensive.org
MIT License
440 stars 38 forks source link

fix(react): improved useTimeout and added usePreservedCallback #966

Closed ssi02014 closed 3 weeks ago

ssi02014 commented 3 weeks ago

Overview

@manudeli Hello! 👋

Currently, useTimeout hook is written to call the changed function when the callback function changes. However, in practice, we're not actually calling the changed function, even if the function changes in the middle.

This is because the only value we currently depend on for useEffect is ms. Adding ref to the dependency array doesn't solve this problem because it's not tracked.

These problems can be solved by using hooks like usePreservedCallback in toss/slash, which can preserve the reference of the callback function, while at the same time ensuring the function is up-to-date.

Since the ref keeps updating during its lifecycle, when calling the function returned by useCallback, the latest function will be called by the closure.

PR Checklist

  1. I read the Contributing Guide
  2. I added documents and tests.
changeset-bot[bot] commented 3 weeks ago

🦋 Changeset detected

Latest commit: 4cb88a46d9b3c437e8de2b5cfe3f744422c90d95

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages | Name | Type | | ----------------------- | ----- | | @suspensive/react | Patch | | @suspensive/react-query | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 3 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 9:35am
v1.suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 9:35am
visualization.suspensive.org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 24, 2024 9:35am
codspeed-hq[bot] commented 3 weeks ago

CodSpeed Performance Report

Merging #966 will create unknown performance changes

Comparing ssi02014:feat/usePreservedCallback (4cb88a4) with main (ec57398)

Summary

:warning: No benchmarks were detected in both the base of the PR and the PR.

codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.88%. Comparing base (ec57398) to head (4cb88a4).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/toss/suspensive/pull/966/graphs/tree.svg?width=650&height=150&src=pr&token=5PopssACmx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss)](https://app.codecov.io/gh/toss/suspensive/pull/966?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) ```diff @@ Coverage Diff @@ ## main #966 +/- ## ========================================== + Coverage 80.71% 80.88% +0.17% ========================================== Files 37 38 +1 Lines 446 450 +4 Branches 99 99 ========================================== + Hits 360 364 +4 Misses 77 77 Partials 9 9 ``` | [Components](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | Coverage Δ | | |---|---|---| | [@suspensive/react](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `97.03% <100.00%> (+0.04%)` | :arrow_up: | | [@suspensive/react-query](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `∅ <ø> (∅)` | | | [@suspensive/react-query-4](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `0.00% <ø> (ø)` | | | [@suspensive/react-query-5](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `0.00% <ø> (ø)` | | | [@suspensive/react-await](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `100.00% <ø> (ø)` | | | [@suspensive/react-image](https://app.codecov.io/gh/toss/suspensive/pull/966/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) | `23.52% <ø> (ø)` | |
ssi02014 commented 3 weeks ago

@manudeli

To make how to work be same between AS-IS and TO-BE, we should add useCallback in all components using useTimeout. There is no significant difference in components that use useTimeout, but it can be expected that performance will be slightly worse. So I'm confuse that this is really improvement. I need more reasons to merge this change

I did this because what was already written wasn't working as intended 🤔 If you don't want to guarantee latest function, the code below should be removed because it's pointless.

import { useEffect, useRef } from 'react'
import { useIsomorphicLayoutEffect } from './useIsomorphicLayoutEffect'

export const useTimeout = (fn: () => void, ms: number) => {
-  const fnRef = useRef(fn)

-  useIsomorphicLayoutEffect(() => {
-    fnRef.current = fn
-  }, [fn])

  useEffect(() => {
-    const id = setTimeout(fnRef.current, ms)
+    const id = setTimeout(fn, ms)
    return () => clearTimeout(id)
  }, [ms])
}

++ Even if you don't use usePreservedCallback and want to guarantee that your function is up-to-date, you shouldn't put "fn" in the dependency array of useEffect because there is an infinite call issue.

export const usePreservedCallback = <T extends (...args: unknown[]) => unknown>(callback: T) => {
  const callbackRef = useRef<T>(callback)

  useIsomorphicLayoutEffect(() => {
    callbackRef.current = callback
  }, [callback])

  return callbackRef.current // (*)
}

++ If you return ref.current directly without using useCallback as above, this is also problematic because it doesn't return the latest function.

ssi02014 commented 3 weeks ago

@manudeli I recently took a deep dive into the usePreservedCallback hook. As a result of this reflection, I'm happy to contribute to a great library once again. 🤗🚀