-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Hooks Testing 🎣 #274
Hooks Testing 🎣 #274
Conversation
Nice! Some things off the top of my head that would be nice to add:
|
Implementation looks good.
|
This will need to be bumped to the non-alpha in a few weeks when Hooks lands in 16.8.0
I've added This will need to be bumped to the non-alpha in a few weeks when Hooks lands in 16.8.0. So... this PR should be held and not merged until Hooks lands in production. |
README updated with link to |
package.json
Outdated
"react": "^16.5.2", | ||
"react-dom": "^16.5.2", | ||
"react": "16.8.0-alpha.1", | ||
"react-dom": "16.8.0-alpha.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we put ^16.5.2 || 16.8.0-alpha.1
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it resolves 16.7.0 (no pre-release) https://semver.npmjs.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version here will have to be something that gets us hooks otherwise our tests will fail. Our peerDependencies
thing is what matters most anyway.
@kentcdodds @donavon what do you think of removing the examples folder in favor of the pages in the docs with the same code, and the Codesandbox? Then we don't have to worry about the package version or dependencies for the examples. Another idea is to make a package.json for the examples folder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like hooks are getting released soon so I'm fine waiting to merge this until hooks are stable (then we can update the devDependency to reference a stable react version).
package.json
Outdated
"react": "^16.5.2", | ||
"react-dom": "^16.5.2", | ||
"react": "16.8.0-alpha.1", | ||
"react-dom": "16.8.0-alpha.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version here will have to be something that gets us hooks otherwise our tests will fail. Our peerDependencies
thing is what matters most anyway.
/** | ||
* Renders a test component that calls back to the test. | ||
*/ | ||
export function testHook(callback: () => void): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do anything special to indicate that the callback will be called with the result of the custom hook (so you can get type safety there) or is TypeScript's inference good enough to know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the callback isn't called with anything and isn't required to return anything. i'm not a TS expert, so I'll defer, but this should work.
Oh, also, THANK YOU A TON @donavon! |
Of course @kentcdodds. I think we should be ready to release I'll work to get things tied up with the PR. Then, when we're satisfied, I'll pre-update the dependencies to say 16.8.0. This will, of course, fail in Travis as there isn't a 16.8.0 today. But the second 16.8.0 drops, all you have to do it re-run the job in Travis (it should pass, hopefully anyway 🤞) then merge the PR. However, I'm on PTO starting Monday and don't think I'll have the time between now and then to write the |
Better get this ready 🚦😉 |
That all sounds fine @donavon 👍 |
NOTE: Travis should FAIL with this commit. Re-run the job when React 16.8.0 drops (soon).
dependencies updated and Travis build failed (as it should). with this, we should be ready to go! my work is done here. 😉 let me know if you need anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM..
@donavon Thank you so much for this PR. The code looks good to me, as Kent mentioned, we can fix some nits and get this asap when hooks are available! 👍 |
We should check out the new test utils API in reactjs/react.dev#1629 |
The React team consulted with me about this. As soon as the new version lands I'll personally take on the work of updating things to work with hooks and their new APIs. |
Ok great @kentcdodds 👍 |
Changed this to |
What happened to this feature? Unfortunately, I cannot find it anywhere... |
👋 you’ll wanna check out react-hooks-testing-library if you’re testing custom hooks that you share between components. Otherwise, testing hooks via user interactions like clicks, etc. works just like anything else would with this library. Good luck! |
We should finish the PR to add that lib to the docs |
What:
Add a
testHook
utilityWhy:
You can't call a Hook directly as it must run in context of a React component.
How:
testHook
creates a component under-the-hood that calls back to the test where you can safely call the Hook. This so that the Hook can run within the context of a component, but set local deconstructed variables.Here is an example of how to test a counter component.
(see above test running live in CodeSandbox)
Any type of return types are supported in
hookTester
: objects, arrays, string, etc.See discussion on #261 for more details.
Checklist: