fix(react): improved useTimeout and added usePreservedCallback

[ssi02014]

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.

• usePreservedCallback

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

• I did below actions if need

1. I read the Contributing Guide
2. I added documents and tests.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4cb88a4

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]

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]

CodSpeed Performance Report

Merging #966 will create unknown performance changes

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

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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	Coverage Δ
@suspensive/react	97.03% <100.00%> (+0.04%)	⬆️
@suspensive/react-query	∅ <ø> (∅)
@suspensive/react-query-4	0.00% <ø> (ø)
@suspensive/react-query-5	0.00% <ø> (ø)
@suspensive/react-await	100.00% <ø> (ø)
@suspensive/react-image	23.52% <ø> (ø)

[ssi02014]

This is slightly different from toss/slash's usePreservedCallback.

I used useIsomorphicLayoutEffect instead of useEffect, and changed the type to unknown instead of any.

[ssi02014]

If it's existing code, it won't call the changed function as intended, and the test will fail. 😭

[manudeli]

1. @suspensive/test-utils should be changed like this implementation.
2. 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

[manudeli]

I think Components(Delay) using useTimeout should use useCallback too.

[manudeli]

@ssi02014 I absolutely accept your opinion #966 (comment) but anyway

I think Components(Delay) using useTimeout should use useCallback too. doesn't it right?

TO-BE

import { type PropsWithChildren, type ReactNode, useContext, useState } from 'react'
import { DelayDefaultPropsContext } from './contexts'
import { useTimeout } from './hooks'
import { Message_Delay_ms_prop_should_be_greater_than_or_equal_to_0, SuspensiveError } from './models/SuspensiveError'

export interface DelayProps extends PropsWithChildren {
  ms?: number
  fallback?: ReactNode
}

export const Delay = (props: DelayProps) => {
  if (process.env.NODE_ENV === 'development') {
    if (typeof props.ms === 'number') {
      SuspensiveError.assert(props.ms >= 0, Message_Delay_ms_prop_should_be_greater_than_or_equal_to_0)
    }
  }
  const defaultProps = useContext(DelayDefaultPropsContext)
  const ms = props.ms ?? defaultProps.ms ?? 0

  const [isDelaying, setIsDelaying] = useState(ms > 0)
-  useTimeout(() => setIsDelaying(false), ms)
+  useTimeout(useCallback(() => setIsDelaying(false), []), ms)

  const fallback = typeof props.fallback === 'undefined' ? defaultProps.fallback : props.fallback
  return <>{isDelaying ? fallback : props.children}</>
}
if (process.env.NODE_ENV === 'development') {
  Delay.displayName = 'Delay'
}

[ssi02014]

@manudeli Oh no, useTimeout works the same way.
No need to change it! 👍

In usePreservedCallback, the callback is wrapped in useCallback, so there is no difference in usage.

 useTimeout(() => setIsDelaying(false), ms)

[manudeli]

Ah! I understood what you mean! Thanks

[ssi02014]

@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.

[manudeli]

Much better!

[ssi02014]

@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. 🤗🚀