Search code, repositories, users, issues, pull requests...

Continue to review full report at Codecov.

Copy link

Contributor

tigerabrodi commented Dec 9, 2020

Glad to have you. Feel free to ask any questions and push up any changes for a review before it's ready.

I look forward to seeing what you all come up with.

@mpeyper Luckily Batman is here, in case an emergency happens 😝

@nobrayner nobrayner force-pushed the master branch 2 times, most recently from 9def482 to b7c28dc Compare

December 9, 2020 22:28

@codecov

Copy link

codecov bot commented Dec 9, 2020 •

edited

Loading

Codecov Report

Merging #515 (1779a85) into master (c53b56b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@
## master #515 +/- ##
=========================================
 Coverage 100.00% 100.00% 
=========================================
 Files 4 4 
 Lines 127 123 -4 
 Branches 24 23 -1 
=========================================
- Hits 127 123 -4

Impacted Files	Coverage Δ
src/index.ts	`100.00% <ø> (ø)`
src/asyncUtils.ts	`100.00% <100.00%> (ø)`
src/cleanup.ts	`100.00% <100.00%> (ø)`
src/pure.tsx	`100.00% <100.00%> (ø)`

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c53b56b...1779a85. Read the comment docs.

merodiro

merodiro reviewed

Dec 9, 2020

src/cleanup.ts Outdated

@@ -1,4 +1,4 @@

let cleanupCallbacks = []

let cleanupCallbacks: (() => Promise<void>)[] = []

Copy link

Contributor

@merodiro merodiro Dec 9, 2020

Choose a reason for hiding this comment

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

I think the return type of callbacks should be Promise<void>|void in case the callback is not async

Copy link

Contributor

@nobrayner nobrayner Dec 9, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

@merodiro I had done it this way, as the callbacks are always called with await - Does it still make sense to be Promise<void> | void even if the callbacks are always called with await?

Copy link

Contributor

@merodiro merodiro Dec 10, 2020

Choose a reason for hiding this comment

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

await can still work if the code is synchronous you can see an example in the documentation for the await operator.
so it makes sense to use await in this case because it may or may not be a promise

Copy link

Contributor

@nobrayner nobrayner Dec 10, 2020

Choose a reason for hiding this comment

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

@merodiro Thanks! Fixing that now...

Screenshot from 2020年12月10日 21-44-51

Copy link

Contributor

tigerabrodi commented Dec 10, 2020 •

edited

Loading

When handling errors in asyncUtils, at two places I need to check if timeout does exist in unknown, how can I go about doing this?

Do you have any ideas guys, @TkDodo & @eps1lon, your input on this?

Edit: This was solved by using instanceof TimeoutError which we already have 🎉

JacobMGEvans reviewed

Dec 10, 2020

Copy link

Contributor

@JacobMGEvans JacobMGEvans left a comment •

edited

Loading

Choose a reason for hiding this comment

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

Progress is looking awesome!!
Only been a day or so! Crushing it!

Copy link

Contributor

JacobMGEvans commented Dec 11, 2020 •

edited

Loading

@kentcdodds @mpeyper I was hoping to get input on this. Currently, if I remove the undefined type which would only allow the user to return something in the waitFor(), there are tests that are structured in a way that would fail. The questions would be, how opinionated does the library want to be in how users employ the utility. I also currently have these types on the T | Promise<T> | undefined | VoidFunction
Screen Shot 2020年12月10日 at 7 36 23 PM

Copy link

Contributor

JacobMGEvans commented Dec 11, 2020 •

edited

Loading

@kentcdodds @mpeyper I was hoping to get input on this. Currently, if I remove the undefined type which would only allow the user to return something in the waitFor(), there are tests that are structured in a way that would fail. The questions would be, how opinionated does the library want to be in how users employ the utility. I also currently have these types on the T | Promise<T> | undefined | VoidFunction
Screen Shot 2020年12月10日 at 7 36 23 PM

@merodiro noticed if we utilize the OR operator with the types we wanted to pass in the possible desired behavior we wanted would work with the current tests.

JacobMGEvans reviewed

Dec 11, 2020

src/asyncUtils.ts Outdated

Comment on lines 48 to 64

// TODO: Discuss with Kent and Maintainers about behavior of returning nothing currently there are tests handling this behavior that may be an anti-pattern.

// ? Should waitFor() always expect something returned

const waitFor = async <T>(

callback: () => T | Promise<T>,

{ interval, timeout, suppressErrors = true }: WaitOptions = {}

) => {

const checkResult = () => {

try {

const callbackResult = callback()

return callbackResult || callbackResult === undefined

} catch (e) {

} catch (error: unknown) {

if (!suppressErrors) {

throw e

throw error as Error

}

return undefined

}

Copy link

Contributor

@JacobMGEvans JacobMGEvans Dec 11, 2020

Choose a reason for hiding this comment

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

@kentcdodds @mpeyper This is the current Types for waitFor() the type is currently expecting "something" to be returned. We could make it more explicit to never expect void null etc...

Copy link

Member

@mpeyper mpeyper Dec 11, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

There are two use cases for waitFor in this library (I'm not sure how it behaves in RTL)

waitFor this thing to not throw:
```
waitFor(() => {
 expect(result.current).toBe(expectedValue)
})
```
In this case, the only value that would break the waiting is if the callback returns undefined (no return).
waitFor this thing to return truthy
```
waitFor(() => result.current === expectedValue)
```
In this case, the wait will only break when the true (or any thruthy value) is returned.

Does that help?

Copy link

Contributor

@JacobMGEvans JacobMGEvans Dec 11, 2020

Choose a reason for hiding this comment

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

Definitely helps! Thanks.

JacobMGEvans reviewed

Dec 11, 2020

Copy link

Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

Minimum types started for Pure file
File needs better, improved typing
Refactoring needed for ESLint Disables to be removed IF POSSIBLE

JacobMGEvans reviewed

Dec 11, 2020

src/pure.tsx Outdated

act(() => {

update(toRender())

})

}

function unmountHook() {

// eslint-disable-next-line @typescript-eslint/no-floating-promises

act(() => {

removeCleanup(unmountHook)

unmount()

Copy link

Contributor

@JacobMGEvans JacobMGEvans Dec 11, 2020

Choose a reason for hiding this comment

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

The Pure file needs to be heavily reviewed and ensure that the types are ideal for user interfacing.

Copy link

Contributor

JacobMGEvans commented Dec 12, 2020 •

edited

Loading

It's gonna pass... 😅 lol

EDIT: Victory!! lol

JacobMGEvans reviewed

src/pure.tsx Outdated

@@ -48,7 +52,7 @@ function resultContainer() {

}

const updateResult = (value: unknown, error?: unknown) => {

const updateResult = (value?: R, error?: Error) => {

Copy link

Contributor

@JacobMGEvans JacobMGEvans Dec 12, 2020

Choose a reason for hiding this comment

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

Nice!

Copy link

Contributor

JacobMGEvans commented Dec 12, 2020

previous commit: All tests are strongly typed with minimal types
to allow for working and made sure tests types were easily usable
with types in Pure and Utils file, allow for good UX & DX

Copy link

Contributor

JacobMGEvans commented Dec 12, 2020

@mpeyper This is likely ready for review.

Copy link

Contributor

tigerabrodi commented Dec 12, 2020

Well done heroes! The villain is soon defeated 🎉

Just gotta wait for Batman 😁 🙌

@mpeyper

Copy link

Member

mpeyper commented Dec 12, 2020

Thanks so much for the effort!

I'll give this a review tonight, which is about 13 hours away in my timezone.

merodiro

merodiro reviewed

src/pure.tsx Outdated

Comment on lines 9 to 15

type Props<T = any, R = any> = {

callback: (props: T) => R

hookProps: unknown

onError: CallableFunction

children: CallableFunction

}

function TestHook({ callback, hookProps, onError, children }: Props) {

Copy link

Contributor

@merodiro merodiro Dec 12, 2020

Choose a reason for hiding this comment

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

I'm not sure if this needs to be generic but we are not calling passing anything to it so it will always be any

Copy link

Contributor

@nobrayner nobrayner Dec 12, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

@merodiro It should be inferred from the callback wich is passed on line 69:
image

Agree that it probably shouldn't be any, though...

Copy link

Contributor

@merodiro merodiro Dec 12, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

<TestHook> isn't generic and isn't passing anything to Props on line 15 so it will always be any

Copy link

Contributor

@nobrayner nobrayner Dec 13, 2020

Choose a reason for hiding this comment

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

@merodiro Oh 🤦‍♂️ I completely missed that

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

I think TestHook should be made generic.

merodiro

merodiro reviewed

src/pure.tsx Outdated

}

// eslint-disable-next-line @typescript-eslint/no-explicit-any

function renderHook<T = any, R = any>(

Copy link

Contributor

@merodiro merodiro Dec 12, 2020

Choose a reason for hiding this comment

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

I think we can remove the default value (any) because it will be inferred

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

Agreed that these should ideally be inferred in s many situations as possible.

I'm a bit worried about the scenario where initialProps is not provided in the renderHooks call, but new props are provided in the rerender call, e.g.

test('should infer from initialProps', () => {
 const { rerender } = renderHook(({ arg }) => useSomeHook(arg), {
 initialProps: { arg: false }
 })
 rerender({ arg: true })
})
test('should infer from defaulted args', () => {
 const { rerender } = renderHook(({ arg = false } = {}) => useSomeHook(arg))
 rerender({ arg: true })
})

(note: the inferring from the defaulted args is the ideal goal, but might not actually have enough information to infer successfully, even with the best typings in the world)

nobrayner

nobrayner reviewed

src/pure.tsx Outdated

import { cleanup, addCleanup, removeCleanup } from './cleanup'

// TODO: Add better type, currently file is utilizing minimum types to work

// TODO: Attempt to refactor code to remove ESLint disables if possible

Copy link

Contributor

@nobrayner nobrayner Dec 12, 2020

Choose a reason for hiding this comment

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

We probably don't need these TODO lines anymore?

Copy link

Contributor

@JacobMGEvans JacobMGEvans Dec 13, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

We do not, please remove them.

EDIT: I was able to remove it. 😄

JacobMGEvans reviewed

Dec 13, 2020

src/pure.tsx Outdated Show resolved Hide resolved

mpeyper

mpeyper reviewed

Dec 13, 2020

Copy link

Member

@mpeyper mpeyper left a comment

Choose a reason for hiding this comment

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

I haven't reviewed asyncUtils yet, but ran out of time tonight. Other than the comments I've left a few other notes:

running npm run build produces types into nested directories in the lib directory. This is because of the seperated test root. I'm happy if you want to move them into src/__tests__ to make the issue go away.
you haven't defined a types field in package.json. This is required if the generated types are no at /index.d.ts of the repo's root (kcd-scripts does not output them there). I think lib/index.d.ts will work if you do 1 of this list
There's a lot of eslint rules being disabled. Generally I'm against doing this on a per-line basis. either the rule is bogus for this project (like the dangling promise one and act) and should be turned off globally, or the issue should actually be addressed.

I'll continue with this tomorrow.

test/suspenseHook.ts Outdated

@@ -1,10 +1,10 @@

import { renderHook } from '../src'

describe('suspense hook tests', () => {

const cache = {}

const fetchName = (isSuccessful) => {

const cache: { value?: Promise<string> | string | Error } = {}

Copy link

Member

@mpeyper mpeyper Dec 13, 2020 •

edited

Loading

Choose a reason for hiding this comment

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

Running npm run validate produces:

[lint] /Users/mxp001/programming/frontend/libraries/react-hooks-testing-library/test/suspenseHook.ts
[lint] 17:24 warning Unsafe assignment of an any value @typescript-eslint/no-unsafe-assignment

Making this const cache: { value?: Promise<string | Error> | string | Error } = {} and .catch((e: Error) => (cache.value = e)) on line 17 removes the warning.

Copy link

Contributor

@tigerabrodi tigerabrodi Dec 13, 2020

Choose a reason for hiding this comment

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

Has been fixed 🎉

src/cleanup.ts Outdated

@@ -7,12 +7,12 @@ async function cleanup() {

cleanupCallbacks = []

}

function addCleanup(callback) {

function addCleanup(callback: () => Promise<void> | void) {

cleanupCallbacks.unshift(callback)

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

This isn't related to the PR, but I wonder if this would be clearer as cleanupCallbacks = [callback, ...cleanupCallbacks] and also align better with the other immutable updates of cleanupCallbacks in this file?

Copy link

Contributor

@tigerabrodi tigerabrodi Dec 13, 2020

Choose a reason for hiding this comment

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

Seems like a good one, updated 🎉

src/pure.tsx Outdated

}

// eslint-disable-next-line @typescript-eslint/no-explicit-any

function renderHook<T = any, R = any>(

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

Agreed that these should ideally be inferred in s many situations as possible.

I'm a bit worried about the scenario where initialProps is not provided in the renderHooks call, but new props are provided in the rerender call, e.g.

test('should infer from initialProps', () => {
 const { rerender } = renderHook(({ arg }) => useSomeHook(arg), {
 initialProps: { arg: false }
 })
 rerender({ arg: true })
})
test('should infer from defaulted args', () => {
 const { rerender } = renderHook(({ arg = false } = {}) => useSomeHook(arg))
 rerender({ arg: true })
})

(note: the inferring from the defaulted args is the ideal goal, but might not actually have enough information to infer successfully, even with the best typings in the world)

src/pure.tsx Outdated

Comment on lines 9 to 15

type Props<T = any, R = any> = {

callback: (props: T) => R

hookProps: unknown

onError: CallableFunction

children: CallableFunction

}

function TestHook({ callback, hookProps, onError, children }: Props) {

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

I think TestHook should be made generic.

src/pure.tsx Outdated

hookProps: unknown

onError: CallableFunction

children: CallableFunction

}

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

I think Props would be better as

type TestHookProps<T, R> = {
 callback: (props: T) => R
 hookProps: R
 onError: (error: Error) => void
 children: (value: R) => void
}

src/pure.tsx Outdated

import { cleanup, addCleanup, removeCleanup } from './cleanup'

// eslint-disable-next-line @typescript-eslint/no-explicit-any

type Props<T = any, R = any> = {

Copy link

Member

@mpeyper mpeyper Dec 13, 2020

Choose a reason for hiding this comment

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

In general, I prefer generic types to better describe what they are genericizing, e.g. TProps and TResult make following their usage (and intellisense) much easier to me.

Copy link

Contributor

tigerabrodi commented Dec 13, 2020

@mpeyper Thanks Batman 🥰, will continue fighting the villains 😄, I promise you 😤! 💪

I will take a look at this now and try to do what I can, I will also take a second look at asyncUtils, perhaps I find something in there, that could be bettered 🎉

mpeyper

mpeyper reviewed

Dec 14, 2020

Copy link

Member

@mpeyper mpeyper left a comment

Choose a reason for hiding this comment

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

asyncUtils is looking ok. Still more disabled lint rules than I generally like. I may have to be a bit lenient on them though given the codebase was never really designed with TS in mind. We can always work on them piecemeal after these changes are merged.

One more thing: I noticed we have a reference to the DefinitelyTyped types in our contributing guide. That probably needs to reworded in some way or just removed completely as well as it won't be relevant once this is merged.

src/asyncUtils.ts Outdated

timeout = true

}

function createTimeoutError(utilName: string, { timeout }: Pick<WaitOptions, 'timeout'>) {

Copy link

Member

@mpeyper mpeyper Dec 14, 2020

Choose a reason for hiding this comment

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

could this just be a constructor on TimeoutError?

src/asyncUtils.ts Outdated

timeoutId = setTimeout(

let timeoutId: number

if (options.timeout && options.timeout > 0) {

timeoutId = window.setTimeout(

Copy link

Member

@mpeyper mpeyper Dec 14, 2020

Choose a reason for hiding this comment

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

why is window required here but not on line 20?

src/asyncUtils.ts Outdated

@@ -80,40 +91,22 @@ function asyncUtils(addResolver) {

}

const waitForValueToChange = async (selector, options = {}) => {

const waitForValueToChange = async (selector: () => unknown, options = {}) => {

Copy link

Member

@mpeyper mpeyper Dec 14, 2020

Choose a reason for hiding this comment

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

I think options should be typed to WaitOptions here to have better support for consumers.

src/asyncUtils.ts Outdated

}

class TimeoutError extends Error {

timeout = true

Copy link

Member

@mpeyper mpeyper Dec 14, 2020

Choose a reason for hiding this comment

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

I don't think this field is being used anymore now that error instanceof TimeoutError works

JacobMGEvans reviewed

Dec 14, 2020