Skip to content

Upstream useSafeTimeout & add tests - #1041

Merged
emplums merged 34 commits into
mainfrom
upstream-behaviors
Feb 16, 2021
Merged

Upstream useSafeTimeout & add tests#1041
emplums merged 34 commits into
mainfrom
upstream-behaviors

Conversation

@emplums

@emplumsemplums commented Feb 9, 2021•
edited
Loading

Copy link
Copy Markdown

This PR upstreams the useSafeTimeouts hook and adds some simple tests.

I've also added documentation, including adding a new Hooks subsection to the documentation site 🎉 We might want to rethink how we structure the sidebar later but I think this is fine for now.

Related issue: #1013

Merge checklist

  • Added or updated TypeScript definitions (index.d.ts) if necessary
  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot

changeset-botBot commented Feb 9, 2021•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0a392a0

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

This PR includes changesets to release 1 package
NameType
@primer/componentsMinor

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

vercelBot commented Feb 9, 2021•
edited
Loading

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/q1oh7cujf
✅ Preview: https://primer-components-git-upstream-behaviors.primer.now.sh

@emplumsemplums mentioned this pull request Feb 10, 2021
Closed
7 tasks
emplums
emplums commented Feb 10, 2021
View reviewed changes
Comment threaddocs/content/useSafeTimeout.mdx
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threaddocs/content/useSafeTimeout.mdx Outdated
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadindex.d.ts Outdated
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/hooks/useSafeTimeout.ts
@@ -0,0 +1,33 @@
import {useCallback, useEffect, useRef} from 'react'

type SetTimeout = (handler: TimerHandler, timeout?: number, ...args: any[]) => number

@T-HugsT-HugsFeb 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same suggestion about using typeof window.setTimeout as above.

@emplumsemplumsFeb 11, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried that, but there are some required properties on the native type that doesn't make sense to pass in manually to these wrapper functions:

Property '__promisify__' is missing in type '(handler: TimerHandler, timeout?: number | undefined, ...args: any[]) => number' but required in type 'typeof setTimeout'.ts(2322)

@T-HugsT-HugsFeb 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh weird! Thanks for checking.

T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/hooks/useSafeTimeout.ts Outdated
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/hooks/useSafeTimeout.ts Outdated
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/__tests__/useSafeTimeout.tsx
import useSafeTimeout from '../hooks/useSafeTimeout'

test('should call callback after time', async () => {
const { result, waitFor } = renderHook(() => useSafeTimeout())

@T-HugsT-HugsFeb 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason for not using fake timers on this test?

T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/__tests__/useSafeTimeout.tsx Outdated
T-Hugs
T-Hugs reviewed Feb 10, 2021
View reviewed changes
Comment threadsrc/__tests__/useSafeTimeout.tsx Outdated
@emplums

emplums commented Feb 12, 2021

Copy link
Copy Markdown
Author

Note: I had to do an extensive refactoring of our tests to get them to work nicely with typescript after adding the @testing-library/react-hooks library 😅

@T-HugsT-Hugs mentioned this pull request Feb 12, 2021
Merged
7 tasks
colebemis
colebemis approved these changes Feb 12, 2021
View reviewed changes

@colebemiscolebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work! Thanks for dealing with the tests ❤️ Just left a few minor comments.

Comment threadsrc/__tests__/FilterList.tsx Outdated
Comment threadsrc/__tests__/FilterListItem.tsx Outdated
Comment threadsrc/__tests__/useSafeTimeout.tsx Outdated
Comment threadsrc/utils/testing.tsx Outdated
Comment threadsrc/utils/testing.tsx Outdated
colebemis
colebemis reviewed Feb 12, 2021
View reviewed changes
Comment thread.changeset/spotty-parents-beam.md Outdated
Emily Plummerand others added 6 commits February 16, 2021 12:39
Emily Plummer
Update FilterList.tsx
cc85835
emplums@colebemis
Update src/__tests__/FilterListItem.tsx
b0d2130
Co-authored-by: Cole Bemis <colebemis@github.com>
emplums@colebemis
Update src/__tests__/useSafeTimeout.tsx
ef95b5b
Co-authored-by: Cole Bemis <colebemis@github.com>
emplums@colebemis
Update src/utils/testing.tsx
32924da
Co-authored-by: Cole Bemis <colebemis@github.com>
emplums@colebemis
Update src/utils/testing.tsx
24a2495
Co-authored-by: Cole Bemis <colebemis@github.com>
emplums@colebemis
Update .changeset/spotty-parents-beam.md
0a392a0
Co-authored-by: Cole Bemis <colebemis@github.com>
@emplums
emplums merged commit 1a95178 into mainFeb 16, 2021
@emplums
emplums deleted the upstream-behaviors branch February 16, 2021 20:54
@github-actionsgithub-actionsBot mentioned this pull request Feb 16, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smocklesmockleAwaiting requested review from smockle
2 more reviewers
@T-HugsT-HugsT-Hugs left review comments
@colebemiscolebemiscolebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@emplums@T-Hugs@colebemis