fix(react): define default prop of each components(Delay, Suspense)

[ssi02014]

Overview

I was looking at the Delay component in @suspensive/react and it seems that if we utilize the generic type of PropsWithChildren, we can remove the unnecessary OmitKeyOf utility type.

type PropsWithChildren<P = unknown> = P & { children?: ReactNode | undefined };

Also, I added test code for the fallback of the Delay component.

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: b9348bf

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)
beta	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 8, 2024 2:57am
main	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 8, 2024 2:57am
visualization	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 8, 2024 2:57am

[netlify]

✅ Deploy Preview for suspensive-org ready!

Name	Link
🔨 Latest commit	b9348bf
🔍 Latest deploy log	https://app.netlify.com/sites/suspensive-org/deploys/65c442369185c6000729a18b
😎 Deploy Preview	https://deploy-preview-709--suspensive-org.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[netlify]

✅ Deploy Preview for suspensive-next-streaming-react-query ready!

Name	Link
🔨 Latest commit	b9348bf
🔍 Latest deploy log	https://app.netlify.com/sites/suspensive-next-streaming-react-query/deploys/65c44236aefb970008d48169
😎 Deploy Preview	https://deploy-preview-709--suspensive-next-streaming-react-query.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Merging #709 (b9348bf) into main (5fb61ec) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #709   +/-   ##
=======================================
  Coverage   83.76%   83.76%           
=======================================
  Files          31       31           
  Lines         505      505           
  Branches      116      115    -1     
=======================================
  Hits          423      423           
  Misses         72       72           
  Partials       10       10           

Components	Coverage Δ
@suspensive/react	97.56% <ø> (ø)
@suspensive/react-query	0.00% <ø> (ø)
@suspensive/react-await	100.00% <ø> (ø)
@suspensive/react-image	23.52% <ø> (ø)

[manudeli]

In my opinion, if this is named for meaning props of Delay, because Delay have children prop, we should use PropsWithChildren in DelayProps internally

We can check ErrorBoundaryProps contain type PropsWithChildren internally
https://github.com/bvaughn/react-error-boundary/blob/master/src/types.ts#L18C33-L26

Because of this reason, we had to omit children of each components(Delay, Suspense) intentionally. because we didn't want library users to use default prop for children prop. children prop should be set by use case each time

[ssi02014]

@manudeli Aha I understand the intent, thanks for the quick feedback 🙏

[manudeli]

How about more strictly by timeout option of waitFor?

Suggested change

	await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument())
	await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument(), {
	timeout: ms('0.5s'),
	})

https://testing-library.com/docs/dom-testing-library/api-async/#waitfor

because default timeout is 1000ms

[ssi02014]

@manudeli
I know that option doesn't make much sense if we use a fake timer, but do you think it would be better to add it?

  it('should render fallback content initially and then the actual text after the delay', async () => {
    render(
      <Delay ms={ms('2s')} fallback={<p role="paragraph">fallback</p>}>
        {TEXT}
      </Delay>
    )
    // vi.advanceTimersByTime(ms('1s'))

    expect(screen.queryByRole('paragraph')).toBeInTheDocument()

    // vi.advanceTimersByTime(ms('1s'))

    await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument(), { timeout: ms('2.1s') }) // Success
    // await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument(), { timeout: ms('2s') }) // Failed
  })

In the above case, the timeout of waitFor is valid.

  it('should render fallback content initially and then the actual text after the delay', async () => {
    render(
      <Delay ms={ms('2s')} fallback={<p role="paragraph">fallback</p>}>
        {TEXT}
      </Delay>
    )
    // vi.advanceTimersByTime(ms('1s'))

    expect(screen.queryByRole('paragraph')).toBeInTheDocument()

    // vi.advanceTimersByTime(ms('1s'))

    await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument()) // Failed
  })

However, if we don't use fake time as shown above and don't specify the timeout option, the test will fail because, as you said, the default value of timeout is 1000ms.

  it('should render fallback content initially and then the actual text after the delay', async () => {
    render(
      <Delay ms={ms('2s')} fallback={<p role="paragraph">fallback</p>}>
        {TEXT}
      </Delay>
    )
    vi.advanceTimersByTime(ms('1s'))

    expect(screen.queryByRole('paragraph')).toBeInTheDocument()

    vi.advanceTimersByTime(ms('1s'))

    await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument()) // Success
  })

If we use a fake timer, we don't necessarily need the timeout option - the above test will work fine.

[ssi02014]

I've given a large time value in the example above for illustration purposes (because the default value for timeout is 1000ms) 🙏

[ssi02014]

Reference

https://stackoverflow.com/questions/65970116/react-testing-library-waitfor-not-working-with-settimeout/65970771#65970771?newreg=dcb8efd321ab40cea4e0e0646d9b6934

[manudeli]

In my opinion, fallback should be expose immediately

Suggested change

	vi.advanceTimersByTime(ms('0.5s'))
	expect(screen.queryByRole('paragraph')).toBeInTheDocument()
	vi.advanceTimersByTime(ms('0.5s'))
	expect(screen.queryByRole('paragraph')).toBeInTheDocument()
	vi.advanceTimersByTime(ms('1s'))

[ssi02014]

Fix done!

Additionally, I followed the guidelines in testing-library. The changes are as follows

beforeEach(() => {
  vi.useFakeTimers({ shouldAdvanceTime: true })
})

afterEach(() => {
  vi.runOnlyPendingTimers()
  vi.useRealTimers()
})

Reference

https://testing-library.com/docs/using-fake-timers/

[manudeli]

Thanks! In my opinion, using timeout option of waitFor is optional thing to approve this Pull Request. I think I need to study more about fake timer

[ssi02014]

@manudeli Thanks! 👍