Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 11 commits into from
Feb 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0fa4ea7
refactor(react): improved Delay Type and Add Test Code
ssi02014 Feb 7, 2024
06ef711
fix(react): remove unnecessary async
ssi02014 Feb 7, 2024
eee21aa
Merge branch 'main' into react/Delay
manudeli Feb 8, 2024
84cc2a3
Update packages/react/src/Delay.tsx
ssi02014 Feb 8, 2024
e5353d3
Create swift-foxes-exist.md
Feb 8, 2024
dcfe420
Update packages/react/src/Delay.tsx
ssi02014 Feb 8, 2024
4f6a9ff
Update packages/react/src/contexts/DefaultOptionsContexts.ts
ssi02014 Feb 8, 2024
0b096b6
Update packages/react/src/wrap.tsx
ssi02014 Feb 8, 2024
ce22567
Update .changeset/swift-foxes-exist.md
Feb 8, 2024
2dae595
Create wild-carpets-study.md
Feb 8, 2024
b9348bf
fix(react): fixed test code for the Delay component
ssi02014 Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/swift-foxes-exist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@suspensive/react": patch
---

fix(react): define default prop of each components
5 changes: 5 additions & 0 deletions .changeset/wild-carpets-study.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@suspensive/react": patch
---

test(react): add test case for fallback prop of Delay
28 changes: 25 additions & 3 deletions packages/react/src/Delay.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
import { CustomError, TEXT } from '@suspensive/test-utils'
import { render, screen, waitFor } from '@testing-library/react'
import ms from 'ms'
import { describe, expect, it } from 'vitest'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { Delay } from './Delay'
import { Delay_ms_prop_should_be_greater_than_or_equal_to_0, SuspensiveError } from './models/SuspensiveError'

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

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

describe('<Delay/>', () => {
it('should render the children after the delay', async () => {
render(<Delay ms={ms('0.1s')}>{TEXT}</Delay>)
Expand All @@ -16,9 +25,22 @@ describe('<Delay/>', () => {
render(<Delay>{TEXT}</Delay>)
expect(screen.queryByText(TEXT)).toBeInTheDocument()
})
it('should accept 0 for ms prop', async () => {
it('should accept 0 for ms prop', () => {
render(<Delay ms={0}>{TEXT}</Delay>)
await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument(), { timeout: 1000 })

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

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

await waitFor(() => expect(screen.queryByText(TEXT)).toBeInTheDocument())
Copy link
Member

@manudeli manudeli Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Contributor Author

@ssi02014 ssi02014 Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
  })

스크린샷 2024-02-08 오전 11 36 40

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.

Copy link
Contributor Author

@ssi02014 ssi02014 Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ssi02014 ssi02014 Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference

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

})
it('should throw SuspensiveError if negative number is passed as ms prop', () => {
expect(() => render(<Delay ms={-1}>{TEXT}</Delay>)).toThrow(Delay_ms_prop_should_be_greater_than_or_equal_to_0)
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/contexts/DefaultOptionsContexts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { OmitKeyOf } from '../utility-types'

export const DelayDefaultPropsContext = createContext<OmitKeyOf<DelayProps, 'children'>>({
ms: undefined,
fallback: undefined,
})
if (process.env.NODE_ENV === 'development') {
DelayDefaultPropsContext.displayName = 'DelayDefaultPropsContext'
Expand Down
Loading