Skip to content

Conversation

@joshpensky
Copy link
Contributor

@joshpensky joshpensky commented Mar 27, 2020

Description

Adds test coverage to all hooks

Fixes #3

What This Does

  1. Adds Jest and @testing-library/react-hooks
  2. Adds new tests folder to hold all hook tests
  3. Adds utils/renderHook, a fork of Testing Library's renderer that also includes renderCount

How to Test

npm test

Still Needs

  • useSet test coverage
  • useStateReducer test coverage
  • useMultiRef test coverage

@joshpensky joshpensky added the wip work in progress label Mar 27, 2020
@joshpensky joshpensky self-assigned this Mar 27, 2020
@joshpensky joshpensky removed the wip work in progress label Mar 27, 2020
@joshpensky joshpensky requested review from bchiang7 and gvjacob March 27, 2020 16:50
@joshpensky joshpensky added the testing Related to how tests are conduted label Mar 27, 2020
import { useForceUpdate } from '../src';

describe('useForceUpdate', () => {
test('forces re-render on update', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming the purpose of useForceUpdate is to force rerender of the component that uses it? How will this do that? I think React is smart enough to only update things that rely on state that has been changed. Since only the tick is updating, I don't think the component that uses this hook will rerender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this hook is particularly to be used with hooks that require storing data in refs, but still want to cause an update to the component when the ref value is updated as well

You can see this being used in useMap and useSet. Storing either of the two types in a useState variable and then using the set/add methods don't actually propagate changes to the DOM, since React doesn't know the internal value has been updated

That's where we can use the useForceUpdate method in order to force React to update the UI and recognize a change has occurred

I set up a codebox here for example!: https://codesandbox.io/s/blissful-wood-y817w

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay that makes sense. Definitely out of the scope for this PR but I think useCallback and maybe useReducer could work without the overhead of useForceUpdate. We can revisit this in the future.

@bchiang7
Copy link

@joshpensky This is awesome!

I'm getting typescript errors in some of the tests, like in useMultiRef.test.ts

image

It looks like you added the global.d.ts file like they recommended, so I'm not sure how to fix it 🤔

@bchiang7
Copy link

Also, we used jest-watch-typeahead on MDA to easily run individual files if you're interested! A nice to have for jest

@@ -0,0 +1 @@
module.exports = require('babel-jest').createTransformer();

Choose a reason for hiding this comment

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

What does createTransformer() do?

Copy link
Contributor Author

@joshpensky joshpensky Mar 30, 2020

Choose a reason for hiding this comment

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

I believe the transformer is what allows Jest to transpile all of our TS code to JS so it can run its tests

I'm not sure if this needs to be explicitly stated in its own file anymore, but it's listed under the TS section for Jest! https://jestjs.io/docs/en/getting-started#using-typescript

@joshpensky
Copy link
Contributor Author

@joshpensky This is awesome!

I'm getting typescript errors in some of the tests, like in useMultiRef.test.ts

Hmm yeah I was getting that before and thought I fixed it :/ I went ahead and removed the global.d.ts file and instead imported the jest-extended package to every test file that used it

Hopefully that should fix it! There's still an open issue on this in the jest-extended repo, so we can revisit this when it's resolved!

Copy link

@bchiang7 bchiang7 left a comment

Choose a reason for hiding this comment

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

I definitely need to read this over more than once to understand it all, but from a high level it all looks good!

Would love to take some time to talk over all of this @joshpensky, but I'm sure once there's some documentation it'll be easier for those without context (a.k.a. Brittany who hasn't been in react land for a while) to understand!

@joshpensky joshpensky merged commit 1e078da into master Mar 30, 2020
@joshpensky joshpensky deleted the tests branch March 30, 2020 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to how tests are conduted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add test coverage

4 participants