Convert to TypeScript #515

juhanakristian · 2020-12-09T17:25:12Z

What:

Converting library to TypeScript, issue #498
Removes deprecated wait util (as per discussion in KCD Discord)
https://discord.com/channels/715220730605731931/785649901782433852/786338242101379142

Creating this PR on behalf of @tigerabrodi we and other people from KCD Discord are going to work on it as a group

Checklist:

Documentation updated
Tests
Ready to be merged
Added myself to contributors table

mpeyper · 2020-12-09T19:52:54Z

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.

tigerabrodi · 2020-12-09T20:04:25Z

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 😝

codecov · 2020-12-09T22:58:32Z

Codecov Report

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

@@            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%> (ø)`

Continue to review full report at Codecov.

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 · 2020-12-09T23:09:37Z

src/cleanup.ts

@@ -1,4 +1,4 @@
-let cleanupCallbacks = []
+let cleanupCallbacks: (() => Promise<void>)[] = []


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

@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?

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

@merodiro Thanks! Fixing that now...

tigerabrodi · 2020-12-10T20:50:05Z

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

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

JacobMGEvans · 2020-12-11T01:56:26Z

@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

JacobMGEvans · 2020-12-11T02:05:46Z

@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

@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 · 2020-12-11T02:15:51Z

src/asyncUtils.ts

+  // 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
      }
    }


@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...

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?

Definitely helps! Thanks.

JacobMGEvans

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

JacobMGEvans · 2020-12-11T20:03:40Z

src/pure.tsx

    act(() => {
      update(toRender())
    })
  }

  function unmountHook() {
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises
    act(() => {
      removeCleanup(unmountHook)
      unmount()


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

JacobMGEvans · 2020-12-12T01:26:52Z

It's gonna pass... 😅 lol

EDIT: Victory!! lol

JacobMGEvans · 2020-12-12T14:17:01Z

src/pure.tsx

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

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


JacobMGEvans · 2020-12-12T18:35:26Z

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

JacobMGEvans · 2020-12-12T18:48:39Z

@mpeyper This is likely ready for review.

tigerabrodi · 2020-12-12T20:09:33Z

Well done heroes! The villain is soon defeated 🎉

Just gotta wait for Batman 😁 🙌

mpeyper · 2020-12-12T20:41:41Z

Thanks so much for the effort!

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

merodiro · 2020-12-12T23:29:04Z

src/pure.tsx

+type Props<T = any, R = any> = {
+  callback: (props: T) => R
+  hookProps: unknown
+  onError: CallableFunction
+  children: CallableFunction
+}
+function TestHook({ callback, hookProps, onError, children }: Props) {


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

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

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

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

@merodiro Oh 🤦‍♂️ I completely missed that

I think TestHook should be made generic.

merodiro · 2020-12-12T23:31:54Z

src/pure.tsx

+}
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function renderHook<T = any, R = any>(


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

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 · 2020-12-12T23:45:28Z

src/pure.tsx

+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


We probably don't need these TODO lines anymore?

We do not, please remove them.

EDIT: I was able to remove it. 😄

src/pure.tsx

mpeyper

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.

mpeyper · 2020-12-13T10:57:04Z

test/suspenseHook.ts

@@ -1,10 +1,10 @@
 import { renderHook } from '../src'

 describe('suspense hook tests', () => {
-  const cache = {}
-  const fetchName = (isSuccessful) => {
+  const cache: { value?: Promise<string> | string | Error } = {}


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.

Has been fixed 🎉

mpeyper · 2020-12-13T10:58:56Z

src/cleanup.ts

@@ -7,12 +7,12 @@ async function cleanup() {
  cleanupCallbacks = []
 }

-function addCleanup(callback) {
+function addCleanup(callback: () => Promise<void> | void) {
  cleanupCallbacks.unshift(callback)


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?

Seems like a good one, updated 🎉

mpeyper · 2020-12-13T12:17:09Z

src/pure.tsx

+}
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function renderHook<T = any, R = any>(


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)

mpeyper · 2020-12-13T12:17:45Z

src/pure.tsx

+type Props<T = any, R = any> = {
+  callback: (props: T) => R
+  hookProps: unknown
+  onError: CallableFunction
+  children: CallableFunction
+}
+function TestHook({ callback, hookProps, onError, children }: Props) {


I think TestHook should be made generic.

mpeyper · 2020-12-13T12:19:21Z

src/pure.tsx

+  hookProps: unknown
+  onError: CallableFunction
+  children: CallableFunction
+}


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 }

mpeyper · 2020-12-13T12:21:19Z

src/pure.tsx

+import { cleanup, addCleanup, removeCleanup } from './cleanup'
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type Props<T = any, R = any> = {


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.

tigerabrodi · 2020-12-13T12:38:38Z

@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

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.

mpeyper · 2020-12-14T02:35:51Z

src/asyncUtils.ts

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


could this just be a constructor on TimeoutError?

mpeyper · 2020-12-14T02:38:02Z

src/asyncUtils.ts

-          timeoutId = setTimeout(
+        let timeoutId: number
+        if (options.timeout && options.timeout > 0) {
+          timeoutId = window.setTimeout(


why is window required here but not on line 20?

mpeyper · 2020-12-14T02:49:16Z

src/asyncUtils.ts

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

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


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

mpeyper · 2020-12-14T02:50:06Z

src/asyncUtils.ts

+}
+
+class TimeoutError extends Error {
+  timeout = true


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

JacobMGEvans · 2020-12-14T03:27:11Z

src/pure.tsx

+type TestHookProps<TProps, TResult> = {
+  callback: (props: TProps) => TResult
+  hookProps: TProps | undefined
+  onError: (error: Error) => void
+  children: (value: TResult) => void


Awesome follow-up!

marcosvega91 · 2020-12-14T09:10:26Z

@mpeyper sorry I have closed it with a wrong force push. I asked @juhanakristian to open it again

mpeyper mentioned this pull request Dec 9, 2020

Convert to TypeScript #498

Closed

nobrayner force-pushed the master branch 2 times, most recently from 9def482 to b7c28dc Compare December 9, 2020 22:28

merodiro reviewed Dec 9, 2020

View reviewed changes

JacobMGEvans reviewed Dec 10, 2020

View reviewed changes

JacobMGEvans reviewed Dec 11, 2020

View reviewed changes

JacobMGEvans reviewed Dec 12, 2020

View reviewed changes

merodiro reviewed Dec 12, 2020

View reviewed changes

nobrayner reviewed Dec 12, 2020

View reviewed changes

JacobMGEvans reviewed Dec 13, 2020

View reviewed changes

src/pure.tsx Outdated Show resolved Hide resolved

mpeyper reviewed Dec 13, 2020

View reviewed changes

mpeyper reviewed Dec 14, 2020

View reviewed changes

JacobMGEvans reviewed Dec 14, 2020

View reviewed changes

marcosvega91 closed this Dec 14, 2020

marcosvega91 force-pushed the master branch from 1779a85 to c53b56b Compare December 14, 2020 08:55

		@@ -1,4 +1,4 @@
		let cleanupCallbacks = []
		let cleanupCallbacks: (() => Promise<void>)[] = []

Convert to TypeScript #515

Convert to TypeScript #515

Conversation

juhanakristian commented Dec 9, 2020 • edited Loading

mpeyper commented Dec 9, 2020

tigerabrodi commented Dec 9, 2020

codecov bot commented Dec 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nobrayner Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigerabrodi commented Dec 10, 2020 • edited Loading

JacobMGEvans left a comment • edited Loading

Choose a reason for hiding this comment

JacobMGEvans commented Dec 11, 2020 • edited Loading

JacobMGEvans commented Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

mpeyper Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobMGEvans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobMGEvans commented Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

JacobMGEvans commented Dec 12, 2020

JacobMGEvans commented Dec 12, 2020

tigerabrodi commented Dec 12, 2020

mpeyper commented Dec 12, 2020

Choose a reason for hiding this comment

nobrayner Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

merodiro Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobMGEvans Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

mpeyper left a comment

Choose a reason for hiding this comment

mpeyper Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigerabrodi commented Dec 13, 2020

mpeyper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosvega91 commented Dec 14, 2020 • edited Loading

juhanakristian commented Dec 9, 2020 •

edited

Loading

codecov bot commented Dec 9, 2020 •

edited

Loading

nobrayner Dec 9, 2020 •

edited

Loading

tigerabrodi commented Dec 10, 2020 •

edited

Loading

JacobMGEvans left a comment •

edited

Loading

JacobMGEvans commented Dec 11, 2020 •

edited

Loading

JacobMGEvans commented Dec 11, 2020 •

edited

Loading

mpeyper Dec 11, 2020 •

edited

Loading

JacobMGEvans commented Dec 12, 2020 •

edited

Loading

nobrayner Dec 12, 2020 •

edited

Loading

merodiro Dec 12, 2020 •

edited

Loading

JacobMGEvans Dec 13, 2020 •

edited

Loading

mpeyper Dec 13, 2020 •

edited

Loading

marcosvega91 commented Dec 14, 2020 •

edited

Loading