perf(react): remove unnecessary useSetTimeout's callback calling #264

manudeli · 2023-10-26T09:47:16Z

Overview

@ssi02014 Thanks for reporting important issue. Could you review this Pull Request too?
I remove unnecessary useSetTimeout's callback calling and I added you as co-author in 79fecd9

PR Checklist

I did below actions if need

I read the Contributing Guide
I added documents and tests.

Co-authored-by: Gromit (전민재) <[email protected]>

changeset-bot · 2023-10-26T09:47:19Z

🦋 Changeset detected

Latest commit: bfd97a9

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 · 2023-10-26T09:47:22Z

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

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 27, 2023 2:07pm
visualization	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 27, 2023 2:07pm

codecov · 2023-10-26T09:49:09Z

Codecov Report

Merging #264 (a81f3f3) into main (de912c4) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head a81f3f3 differs from pull request most recent head bfd97a9. Consider uploading reports for the commit bfd97a9 to get more accurate results

@@            Coverage Diff            @@
##              main      #264   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          802       810    +8     
  Branches       135       136    +1     
=========================================
+ Hits           802       810    +8

Components	Coverage Δ
@suspensive/react	`100.00% <100.00%> (ø)`
@suspensive/react-query	`∅ <ø> (∅)`
@suspensive/react-await	`100.00% <ø> (ø)`

manudeli

I left comments as my intention

manudeli · 2023-10-26T09:51:09Z

configs/test-utils/src/ThrowError.tsx

+const isClient = typeof window !== 'undefined'
+export const useIsomorphicLayoutEffect = isClient ? useLayoutEffect : useEffect
+
+export const useTimeout = (fn: () => void, ms: number) => {


I renamed useSetTimeout to useTimeout, because we use just Timeout not setTimeout

How about we use ms package for this? https://www.npmjs.com/package/ms
It is really good for dx.

I understood that you meant to use ms inside suspensive. But wouldn't it be more useful to use it like the code below? If ms is unnecessary, it can prevent unnecessary dependencies from being installed and unnecessary calculations to be performed. Also, the unit of time desired by the Delay component is quite small. In most cases, I expect to use ms prop for Delay of less than 2000ms, but I am wondering whether it would be a good idea to use ms internally in this situation. If suspensive need to include it, I think it would be a good idea to create ms yourself so that you can include only the necessary parts.

Delay using with ms is okay

import ms from 'ms' <Delay ms={ms('2 days')}> {/* ... */} </Delay>

Delay have to make BREAKING CHANGE if we use ms internally

<Delay time={'2 days'}> // this ms prop have to make BREAKING CHANGE. because '2 days' is not just ms prop {/* ... */} </Delay>

I mean to use it for test-utils, It is internal package not for user.
If not, I agree with you.

Oh that makes sense. I agree ms can be good devDependency to test case like this! Let's add ms as root's devDependency in next Pull Request

manudeli · 2023-10-26T09:51:42Z

configs/test-utils/src/ThrowError.tsx

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


I used useIsomorphicLayoutEffect in this time, thanks to @minsoo-web
I referred to this code of usehooks-ts recommended by @ssi02014 a lot. thanks!👍

manudeli · 2023-10-26T09:52:49Z

configs/test-utils/src/ThrowError.tsx

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


remove unncessary fn calling

manudeli · 2023-10-26T10:05:28Z

@ssi02014 If you okay, please approve this Pull Request

ssi02014

LGTM 👏

manudeli · 2023-10-26T14:28:08Z

Ah, we need to add test case to check there is no unncessary useTimeout's fn calling on rerendering. I will add it to prove this fixing will work

ssi02014 · 2023-10-26T15:03:52Z

@manudeli

Yes, I agree. The old useSetTimeout would fail the test code below. But with the new useTimeout, the test passes.

(The test code below is just a reference example, written in jest instead of vitest. Also, rerender in renderHook didn't work well with the test, so I redefined the component and utilized rerender in render).

const delayTime = 1000;

const TestComponent = () => {
  const [number, setNumber] = useState(0);

  useTimeout(() => {
    setNumber(number + 1);
  }, delayTime);

  return <div>{number}</div>;
};

it('should not be re-called even if the component is re-rendered', () => {
  const { rerender } = render(<TestComponent />);

  expect(screen.getByText('0')).toBeInTheDocument();

  act(() => jest.advanceTimersByTime(delayTime));
  expect(screen.getByText('1')).toBeInTheDocument();

  rerender(<TestComponent />);

  act(() => jest.advanceTimersByTime(delayTime))
  expect(screen.getByText('1')).toBeInTheDocument();
});

manudeli · 2023-10-26T15:09:42Z

@manudeli

Yes, I agree. The old useSetTimeout would fail the test code below. But with the new useTimeout, the test passes.

(The test code below is just a reference example, written in jest instead of vitest. Also, rerender in renderHook didn't work well with the test, so I redefined the component and utilized rerender in render).

const delayTime = 1000;

const TestComponent = () => {
  const [number, setNumber] = useState(0);

  useTimeout(() => {
    setNumber(number + 1);
  }, delayTime);

  return <div>{number}</div>;
};

it('should not be re-called even if the component is re-rendered', () => {
  const { rerender } = renderSetup(<TestComponent />);

  expect(screen.getByText('0')).toBeInTheDocument();

  act(() => jest.advanceTimersByTime(delayTime));
  expect(screen.getByText('1')).toBeInTheDocument();

  rerender(<TestComponent />);

  act(() => jest.advanceTimersByTime(delayTime))
  expect(screen.getByText('1')).toBeInTheDocument();
});

Oh, cool! If you okay, could you make Pull Request to this branch too?

ssi02014 · 2023-10-26T15:18:53Z

@manudeli
Yes! If the current Pull Request reflects, I'll work on the test code to create a new Pull Request! is that okay?

Oh sorry I didn't catch your intent, can I just keep the current pull request and work on it?

manudeli · 2023-10-26T15:32:51Z

@manudeli
Yes! If the current Pull Request reflects, I'll work on the test code to create a new Pull Request! is that okay?

Oh sorry I didn't catch your intent, can I just keep the current pull request and work on it?

After merging this, new Pull Request to add test case also okay! Thanks

minsoo-web

Cool!

minsoo-web · 2023-10-26T15:58:21Z

configs/test-utils/src/ThrowError.tsx

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


Suggested change

const id = setTimeout(() => fnRef.current(), ms)

const id = setTimeout(fnRef.current, ms)

what about remove useless callback function?

@manudeli
please check this review

minsoo-web · 2023-10-26T16:02:05Z

configs/test-utils/src/ThrowError.tsx

+const isClient = typeof window !== 'undefined'
+export const useIsomorphicLayoutEffect = isClient ? useLayoutEffect : useEffect
+
+export const useTimeout = (fn: () => void, ms: number) => {


How about we use ms package for this? https://www.npmjs.com/package/ms
It is really good for dx.

configs/test-utils/src/ThrowError.tsx

minsoo-web · 2023-10-26T16:31:23Z

packages/react/src/hooks/useTimeout.ts

+import { useEffect, useRef } from 'react'
+import { useIsomorphicLayoutEffect } from './useIsomorphicLayoutEffect'
+
+export const useTimeout = (fn: () => void, ms: number) => {


this hook is needable to export ? why don't we import from test-util? it is duplicated

@minsoo-web
I checked and the Delay component is using the existing useSetTimeout.

Since "test-utils" has strong test-related semantics, we should consider whether it's appropriate to export and use the "useTimeout" hook from "test-utils". Personally, I think it's more appropriate to put it in suspensive/react.

Currently, "suspensive/react" has "suspensive/test-utils" as a dependency, so adding "suspensive/react" as a dependency from "suspensive/test-utils" to eliminate duplicate code may cause a circular dependency issue.

Is there a good way to remove duplicate code while preserving the intent of the package?

@minsoo-web ssi02014 commented why same with exactly my intention.
also @suspensive/test-utils is just devDependency of @suspensive/react. so we need to import useTimeout from internal module (@suspensive/react/src/hooks).

If you know alternative way, make Pull Request or add Issue please.

This is simply a suggestion, so I hope you take it lightly.

I thought it would be fun to create a new package related to timer and manage it as a repo.

@timer/react

useTimeout

useInterval

@timer/core

delayPromise

I thought it would be a way to take the package we made and distributed as a new dependency.

minsoo-web

LGTM!

minsoo-web · 2023-10-27T13:52:18Z

configs/test-utils/src/ThrowError.tsx

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


@manudeli
please check this review

manudeli · 2023-10-27T14:04:10Z

I made #265 this issue will be resolved by @ssi02014 after merging this Pull Request

@minsoo-web

fix #267 # Overview  @minsoo-web suggest us to add `ms` in #264 (comment) I added `ms` as dev dependency to test apis related with time ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests.

@ssi02014

fix #263 # Overview  @ssi02014 Thanks for reporting important issue. Could you review this Pull Request too? I remove unnecessary useSetTimeout's callback calling and I added you as co-author in [79fecd9](79fecd9) ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests. --------- Co-authored-by: Gromit (전민재) <[email protected]>

@minsoo-web

fix #267 # Overview  @minsoo-web suggest us to add `ms` in #264 (comment) I added `ms` as dev dependency to test apis related with time ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests.

@ssi02014

fix #263 # Overview  @ssi02014 Thanks for reporting important issue. Could you review this Pull Request too? I remove unnecessary useSetTimeout's callback calling and I added you as co-author in [79fecd9](79fecd9) ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests. ---------

@minsoo-web

fix #267 # Overview  @minsoo-web suggest us to add `ms` in #264 (comment) I added `ms` as dev dependency to test apis related with time ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests.

@ssi02014

fix #263 # Overview  @ssi02014 Thanks for reporting important issue. Could you review this Pull Request too? I remove unnecessary useSetTimeout's callback calling and I added you as co-author in [79fecd9](79fecd9) ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests. --------- Co-authored-by: Gromit (전민재) <[email protected]>

@minsoo-web

fix #267 # Overview  @minsoo-web suggest us to add `ms` in #264 (comment) I added `ms` as dev dependency to test apis related with time ## PR Checklist - [x] I did below actions if need 1. I read the [Contributing Guide](https://github.com/suspensive/react/blob/main/CONTRIBUTING.md) 2. I added documents and tests.

perf(react): remove unnecessary useSetTimeout's callback calling

79fecd9

Co-authored-by: Gromit (전민재) <[email protected]>

github-actions bot added the @suspensive/react label Oct 26, 2023

Create rare-chefs-behave.md

f0cb717

manudeli marked this pull request as ready for review October 26, 2023 09:48

manudeli requested review from okinawaa and minsoo-web as code owners October 26, 2023 09:48

vercel bot temporarily deployed to Preview – docs October 26, 2023 09:49 Inactive

manudeli commented Oct 26, 2023

View reviewed changes

manudeli self-assigned this Oct 26, 2023

ssi02014 approved these changes Oct 26, 2023

View reviewed changes

minsoo-web reviewed Oct 26, 2023

View reviewed changes

manudeli requested a review from minsoo-web October 26, 2023 19:08

minsoo-web approved these changes Oct 27, 2023

View reviewed changes

chore: reflect code review

a81f3f3

vercel bot temporarily deployed to Preview – visualization October 27, 2023 14:00 Inactive

vercel bot temporarily deployed to Preview – docs October 27, 2023 14:00 Inactive

manudeli mentioned this pull request Oct 27, 2023

[Feature] add test case for useTimeout #265

Closed

Merge branch 'main' into react/perf/useTimeout

bfd97a9

vercel bot temporarily deployed to Preview – visualization October 27, 2023 14:06 Inactive

vercel bot temporarily deployed to Preview – docs October 27, 2023 14:07 Inactive

manudeli merged commit c09f52a into main Oct 27, 2023
11 checks passed

manudeli deleted the react/perf/useTimeout branch October 27, 2023 14:07

This was referenced Oct 27, 2023

[Feature] ms package as root devDependency to test time #267

Closed

chore(ms): add dev dependency #274

Merged

manudeli assigned ssi02014 Nov 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(react): remove unnecessary useSetTimeout's callback calling #264

perf(react): remove unnecessary useSetTimeout's callback calling #264

manudeli commented Oct 26, 2023 •

edited

Loading

changeset-bot bot commented Oct 26, 2023 •

edited

Loading

vercel bot commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

manudeli left a comment

manudeli Oct 26, 2023

minsoo-web Oct 26, 2023

manudeli Oct 26, 2023 •

edited

Loading

minsoo-web Oct 27, 2023

manudeli Oct 27, 2023 •

edited

Loading

manudeli Oct 26, 2023 •

edited

Loading

manudeli Oct 26, 2023

manudeli commented Oct 26, 2023

ssi02014 left a comment

manudeli commented Oct 26, 2023

ssi02014 commented Oct 26, 2023 •

edited

Loading

manudeli commented Oct 26, 2023

ssi02014 commented Oct 26, 2023 •

edited

Loading

manudeli commented Oct 26, 2023

minsoo-web left a comment

minsoo-web Oct 26, 2023

minsoo-web Oct 27, 2023

minsoo-web Oct 26, 2023

minsoo-web Oct 26, 2023

ssi02014 Oct 26, 2023 •

edited

Loading

manudeli Oct 26, 2023 •

edited

Loading

minsoo-web Oct 27, 2023 •

edited

Loading

minsoo-web left a comment

minsoo-web Oct 27, 2023

manudeli commented Oct 27, 2023

	const id = setTimeout(() => fnRef.current(), ms)
	const id = setTimeout(fnRef.current, ms)

perf(react): remove unnecessary useSetTimeout's callback calling #264

perf(react): remove unnecessary useSetTimeout's callback calling #264

Conversation

manudeli commented Oct 26, 2023 • edited Loading

Overview

PR Checklist

changeset-bot bot commented Oct 26, 2023 • edited Loading

🦋 Changeset detected

vercel bot commented Oct 26, 2023 • edited Loading

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

manudeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manudeli Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Delay using with ms is okay

Delay have to make BREAKING CHANGE if we use ms internally

Choose a reason for hiding this comment

manudeli Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

manudeli Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manudeli commented Oct 26, 2023

ssi02014 left a comment

Choose a reason for hiding this comment

manudeli commented Oct 26, 2023

ssi02014 commented Oct 26, 2023 • edited Loading

manudeli commented Oct 26, 2023

ssi02014 commented Oct 26, 2023 • edited Loading

manudeli commented Oct 26, 2023

minsoo-web left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssi02014 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

manudeli Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

minsoo-web Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

minsoo-web left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manudeli commented Oct 27, 2023

manudeli commented Oct 26, 2023 •

edited

Loading

changeset-bot bot commented Oct 26, 2023 •

edited

Loading

vercel bot commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

manudeli Oct 26, 2023 •

edited

Loading

Delay have to make BREAKING CHANGE if we use `ms` internally

manudeli Oct 27, 2023 •

edited

Loading

manudeli Oct 26, 2023 •

edited

Loading

ssi02014 commented Oct 26, 2023 •

edited

Loading

ssi02014 commented Oct 26, 2023 •

edited

Loading

ssi02014 Oct 26, 2023 •

edited

Loading

manudeli Oct 26, 2023 •

edited

Loading

minsoo-web Oct 27, 2023 •

edited

Loading