Add support for TypeScript by otofu-square · Pull Request #8 · testing-library/react-hooks-testing-library

otofu-square · 2019-02-28T16:16:22Z

Close #7

Introduce TypeScript type definitions 🎉

Add some packages to devDependencies(typescript, typings-tester, @types/react)
Add index.d.ts to define typings
Add test for index.d.ts with typings-tester
- Inspired by the typing tests in Redux repo

otofu-square · 2019-02-28T16:20:59Z

test/typescript/renderHook.ts

+
+const ThemesContext = createContext(themes)
+
+const useTheme = (initialTheme: InitialTheme = DARK) => {


Add minimum typing tests with the example code in README.md.

I'm not a big fan of that example, but I wanted something that used a few different hooks, so it became a little contrived. Happy for you to come up with something better if you want, or just keep it as is that's easier.

so it became a little contrived

I see.

Happy for you to come up with something better

I'll give it a try soon 👍

Replace useTheme hook with useCounter here 3cb8f92.
Also fix type definitions because I just realized they are broken.

otofu-square · 2019-02-28T16:23:37Z

@mpeyper
Please review this PR and let me know if you have any questions. thx!

otofu-square · 2019-02-28T16:39:43Z

Created minimum TS project with react-hooks-testing-library: https://github.com/otofu-square/react-hooks-testing-library-sandbox

mpeyper · 2019-02-28T23:46:26Z

index.d.ts

+  readonly rerender: (hookProps?: P) => void
+}
+
+export const testHook: typeof renderHook


mpeyper · 2019-02-28T23:52:19Z

I'm reviewing this, but I'm far from a Typescript expert (I don't use it day to day, just created basic type definitions for some of my other libraries before, and not of them are as cool as what you've done here), so I'm sorry for any dumb questions. I'd also like it if someone that does use Typescript regularly could chip in and give me an indiation of whether the types here make sense.

mpeyper · 2019-02-28T23:52:59Z

index.d.ts

+import { cleanup, act, RenderOptions, RenderResult } from 'react-testing-library'
+
+export function renderHook<P extends any, T extends (...args: [P]) => any>(
+  callback: (_: P) => ReturnType<T>,


Where does ReturnType come from?

Nevermind, I found something about it in the TS docs

mpeyper · 2019-03-01T00:03:21Z

This looks great to me. Waaaaaay better than anything I could have done myself. As I said before, I'd love a TS user to put a comment on here with approval before merging.

Also can you please add yourself as a contributor to the README (by running this). I haven't got around to setting up a contributing document yet, so I don't blame you for not knowing this.

If anyone TS users do review this, please submit a PR adding yourself as a contributer as well.

otofu-square · 2019-03-01T00:38:59Z

Also can you please add yourself as a contributor to the README

Oh, sorry! Will work on it soon.

otofu-square · 2019-03-01T01:06:24Z

Will work on it soon.

Done.

If anyone TS users do review this

Let me ask someone I know.

tatsushitoji · 2019-03-01T01:20:33Z

index.d.ts

+  readonly rerender: (hookProps?: P) => void
+}
+
+export const testHook: typeof renderHook


It'll be unnecessary someday, but it 's so smart 👍

tatsushitoji

LGTM, if this PR is the minimum typing or even if it does not 👍
I think that there is no problem to merge.

mpeyper · 2019-03-01T10:37:50Z

index.d.ts

@@ -0,0 +1,18 @@
+import { cleanup, act, RenderOptions, RenderResult } from 'react-testing-library'
+
+export function renderHook<P extends any, R extends any>(


what is the difference between renderHook<P extends any, R extends any> and renderHook<P, R>?

rednerHook<P, R> is the same as renderHook<P extends Object, R extends Object>.
Maybe it's more simple to use <P, R> instead of <P extends any, R extends any> since P and R will be inferred by giving callback & initialProps.

index.d.ts

mpeyper · 2019-03-01T10:45:49Z

How do the typings look when the callback returns void (i.e. a hook that does not actually return anything)? Is it worth having a test around that case? I'm not sure how often that will actually occur in the wild so I'm not too concerned, more curious than anything.

otofu-square · 2019-03-01T12:14:42Z

I'm not sure how often that will actually occur in the wild so I'm not too concerned, more curious than anything.

Currently the typing of result.current which returns from renderHook is void type in this case.
IMHO, I think result.current can be unnecessary in any case if hook returns void.

Is it worth having a test around that case?

Maybe it's worth to test that result.current has void type when hook returns void 🤔

index.d.ts

danielkcz · 2019-03-02T11:22:56Z

index.d.ts

+    current: R
+  }
+  readonly unmount: RenderResult['unmount']
+  readonly rerender: (hookProps?: P) => void


Feels unnecessary to have ? here. If the P is inferred as undefined, it should be ok. Otherwise, it should be required to pass the props if there were some initially to avoid surprising results.

Suggested change

readonly rerender: (hookProps?: P) => void

readonly rerender: (hookProps: P) => void

Looking at the implementation of the rerender function, it seems that hookProps.current is assigned as a default value. (hookProps.current is the same as initialProps)
https://github.com/mpeyper/react-hooks-testing-library/blob/f7cd61035d8fb08cbfac376a5a8da1780708a64e/src/index.js#L26

As long as I see this, we can call rerender function with no argument or initialProps.

I see, so it will use initialProps if none are passed. Makes sense I guess although it feels less explicit and might be confusing for tests.

Correct. Subsequent rerenders will use the previously supplied props until new props are supplied.

The idea is that the props don't change until the hook is rendered with new props, so hooks that use a conditional array (i.e. useEffect, useLayoutEffect, useMemo and useCallback) can be reliably tested.

danielkcz · 2019-03-02T11:25:03Z

package.json

    "@babel/plugin-transform-modules-commonjs": "^7.2.0",
    "@babel/preset-env": "^7.3.4",
    "@babel/preset-react": "^7.0.0",
+    "@types/react": "^16.8.5",


Where does it depend on React types actually? I don't see any reference to it.

Suggested change

"@types/react": "^16.8.5",

It's necessary to use React hooks in the typing test.
https://github.com/mpeyper/react-hooks-testing-library/blob/a16d3a303bfcfdd08ed007e524198b061b89d6ef/test/typescript/renderHook.ts#L1-L23

Co-Authored-By: otofu-square <otofu.square@gmail.com>

mpeyper · 2019-03-03T10:40:37Z

Ok, I feel like all comments have either been addressed or explained so I'm going to merge this.

@FredyC, @tatsushitoji (and anyone else who may have contributed in some way to the PR), please open a PR adding yourself code review contributors by running npm run contributors:add and following the prompts.

otofu-square added 3 commits February 28, 2019 23:33

$ npm i --save-dev typescript typings-tester @types/react

7f7ca2b

Add index.d.ts

ae2d990

Add test for TS types

bbfb0ff

otofu-square commented Feb 28, 2019

View reviewed changes

otofu-square added 2 commits March 1, 2019 07:54

Improve typings to infer initialProps type

1ba1bd9

fix renderHook options type to intersect RenderOptions type

eaee298

mpeyper reviewed Feb 28, 2019

View reviewed changes

mpeyper approved these changes Mar 1, 2019

View reviewed changes

otofu-square added 2 commits March 1, 2019 09:56

simplify typing test & fix type definitions

3cb8f92

$ yarn contributors:add

36604a1

tatsushitoji reviewed Mar 1, 2019

View reviewed changes

tatsushitoji approved these changes Mar 1, 2019

View reviewed changes

mpeyper reviewed Mar 1, 2019

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

otofu-square added 2 commits March 2, 2019 00:53

Remove unnecessary extend

2bdb78a

Add test case for the case hook returns void

b2478e8

danielkcz reviewed Mar 2, 2019

View reviewed changes

Simplify the way to extract first argument type

a16d3a3

Co-Authored-By: otofu-square <otofu.square@gmail.com>

mpeyper merged commit bce403f into testing-library:master Mar 3, 2019


		const ThemesContext = createContext(themes)

		const useTheme = (initialTheme: InitialTheme = DARK) => {

		@@ -0,0 +1,18 @@
		import { cleanup, act, RenderOptions, RenderResult } from 'react-testing-library'

		export function renderHook<P extends any, R extends any>(

	readonly rerender: (hookProps?: P) => void
	readonly rerender: (hookProps: P) => void

Comments

Conversation

otofu-square commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

otofu-square Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otofu-square commented Feb 28, 2019

Uh oh!

otofu-square commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeyper commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeyper commented Mar 1, 2019

Uh oh!

otofu-square commented Mar 1, 2019

Uh oh!

otofu-square commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tatsushitoji left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpeyper commented Mar 1, 2019

Uh oh!

otofu-square commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danielkcz Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otofu-square Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielkcz Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otofu-square Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeyper commented Mar 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

otofu-square commented Feb 28, 2019 •

edited

Loading

otofu-square Feb 28, 2019 •

edited

Loading

otofu-square commented Mar 1, 2019 •

edited

Loading

tatsushitoji left a comment •

edited

Loading

otofu-square commented Mar 1, 2019 •

edited

Loading

danielkcz Mar 2, 2019 •

edited

Loading

otofu-square Mar 3, 2019 •

edited

Loading

danielkcz Mar 2, 2019 •

edited

Loading

otofu-square Mar 3, 2019 •

edited

Loading

mpeyper commented Mar 3, 2019 •

edited

Loading