Skip to content
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

Unable to test hooks that Suspend #27

Closed
ntucker opened this issue Apr 1, 2019 · 12 comments · Fixed by #35
Closed

Unable to test hooks that Suspend #27

ntucker opened this issue Apr 1, 2019 · 12 comments · Fixed by #35
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ntucker
Copy link
Contributor

ntucker commented Apr 1, 2019

  • react-hooks-testing-library version: 0.4.0
  • react-testing-library version: 6.0.3
  • react version: 16.8.6
  • node version: 11.10.0
  • npm (or yarn) version: 1.15.2

Relevant code or config:

import { useResource } from 'rest-hooks';
renderHook(() => {
      article = useResource(CoolerArticleResource.singleRequest(), payload);
      return article;
    })

What you did:

Running unit tests on 'rest-hook's library. Trying to update from react-testing-library - which works fine - to react-hooks-testing-library.

(https://github.com/coinbase/rest-hooks/blob/master/src/react-integration/__tests__/integration.tsx)

What happened:

Invariant Violation: Rendered more hooks than during the previous render.

Reproduction:

https://github.com/coinbase/rest-hooks/blob/update-test/src/react-integration/__tests__/integration.tsx#L167
(run yarn test on that branch)

Problem description:

Impossible to test hooks that use suspense. (This was fixed in React 16.8.2 facebook/react#14821)

Suggested solution:

@ntucker ntucker added the bug Something isn't working label Apr 1, 2019
@mpeyper
Copy link
Member

mpeyper commented Apr 1, 2019

I'd say this was was an unintended side effect of #21, where the intention was to catch errors to assert against. I wonder if just rethrowing the caught value would be enough to get this behaviour back. We probably also don't want to be setting result.error if it is a promise either.

Could you try locating this section, but in the babel output version in the lib directory of the node_module and changing it to

function TestHook({ callback, hookProps, children }) {
  try {
    children(callback(hookProps))
    return null
  } catch (e) {
    if (!e.then) { // we'll probably find a better promise check, but this will do for testing purposes
      children(undefined, e)
    }
    throw e
  }
}

and see if it starts working again. If it does, I'm happy for you to submit this as a PR.

Otherwise, if you don't have time for this, I will take a closer look tonight (it's currently 8:30am my time) when the kids go to bed.

@mpeyper mpeyper added the good first issue Good for newcomers label Apr 1, 2019
@ntucker
Copy link
Contributor Author

ntucker commented Apr 2, 2019

Actually I was looking forward to being able to inspect the result.error so I could simply await it. Perhaps a better approach would be to set result.promise if a promise is thrown?

Before I was setting Suspense and ErrorBoundary in my wrapper just to call a passed in function so I knew when they were hit.

@mpeyper
Copy link
Member

mpeyper commented Apr 2, 2019

I wonder if we can somehow tie it into waitForNextUpdate?

@ntucker
Copy link
Contributor Author

ntucker commented Apr 2, 2019

Changing the provided code caused the react warning to go away. However, my fbmock that's triggered on render of fallback from suspense wasn't being called.

Edit: actually the error comes back when I remove the expect check for the fbmock call.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 2, 2019

Full trace:

Invariant Violation: Rendered more hooks than during the previous render.

      13 |   const url = params ? getUrl(params) : '';
      14 | 
    > 15 |   return useMemo(() => {
         |          ^
      16 |     if (!params) return null;
      17 |     return selectMeta(state, url);
      18 |   }, [state, url]);

      at invariant (node_modules/react-dom/cjs/react-dom.development.js:55:15)
      at updateWorkInProgressHook (node_modules/react-dom/cjs/react-dom.development.js:13092:35)
      at updateMemo (node_modules/react-dom/cjs/react-dom.development.js:13465:14)
      at Object.useMemo (node_modules/react-dom/cjs/react-dom.development.js:13816:16)
      at Object.useMemo (node_modules/react/cjs/react.development.js:1492:21)
      at Object.useMeta (src/react-integration/hooks/useMeta.ts:15:10)
      at useError (src/react-integration/hooks/useResource.ts:16:23)
      at useOneResource (src/react-integration/hooks/useResource.ts:49:3)
      at Object.useResource (src/react-integration/hooks/useResource.ts:172:10)
      at testProvider (src/react-integration/__tests__/integration.tsx:161:17)
      at TestHook (node_modules/react-hooks-testing-library/lib/index.js:35:14)
      at renderWithHooks (node_modules/react-dom/cjs/react-dom.development.js:12938:18)
      at updateFunctionComponent (node_modules/react-dom/cjs/react-dom.development.js:14627:20)
      at beginWork (node_modules/react-dom/cjs/react-dom.development.js:15637:16)
      at performUnitOfWork (node_modules/react-dom/cjs/react-dom.development.js:19312:12)
      at workLoop (node_modules/react-dom/cjs/react-dom.development.js:19352:24)
      at renderRoot (node_modules/react-dom/cjs/react-dom.development.js:19435:7)
      at performWorkOnRoot (node_modules/react-dom/cjs/react-dom.development.js:20342:7)
      at performWork (node_modules/react-dom/cjs/react-dom.development.js:20254:7)
      at performSyncWork (node_modules/react-dom/cjs/react-dom.development.js:20228:3)
      at Object.batchedUpdates$1 [as unstable_batchedUpdates] (node_modules/react-dom/cjs/react-dom.development.js:20443:7)
      at act (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1161:27)
      at Object.rtlAct (node_modules/react-testing-library/dist/act-compat.js:29:10)
      at mockDispatch (src/react-integration/__tests__/integration.tsx:15:7)
      at fetch.then.data (src/state/NetworkManager.ts:72:11)

Somehow definitely seems related to facebook/react#14821 since that's about useMemo()

@mpeyper
Copy link
Member

mpeyper commented Apr 3, 2019

I'm having a little trouble setting up a simple test hook that properly suspends the React rendering.

Basically I have a fake fetch promise that I want to wait for

const fetchName = () =>
    new Promise((resolve) => {
      setTimeout(() => resolve('Bob'), 50)
    })

How would you write a simple hook the suspends until this has resolved?

So far I've got:

  const useFetchName = () => {
    const [name, setName] = useState()

    const fetcher = useMemo(fetchName, [])

    useEffect(() => {
      fetcher.then((theName) => {
        setName(theName)
      })
    }, [])

    if (!name) {
      throw fetcher
    }

    return name
  }

but this ends up calling fetchName lots of times and the test eventually times out (like useMemo is not working at all).

I don't really know what I'm doing with Suspense here, so it could be completely wrong, but my theory was to throw the same promise until it resolves and then return the name.

Any help would be much appreciated.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 3, 2019

You need to throw the promise (the exact same one) until it resolves. Once it resolves you do whatever else. The easiest way is to just use a singleton like react-cache does.

const cache = {}
function get(key) {
  if (!(key in cache)) {
    cache[key] = new Promise((resolve) => {
      setTimeout(() => resolve('Bob'), 50)
    }).then(value => cache[key] = value).catch(e => cache[key] = e);
  }
  return cache[key]
}

const SuspendMe = () => {
  const myvalue = get('stuff');
  if (typeof myvalue.then === 'function') {
    throw myvalue;
  } else if (myvalue instanceof Error) {
    // you'll need an <ErrorBoundary /> above this component in the tree to handle this 
    throw myvalue;
  }
  return myvalue;
}

@mpeyper
Copy link
Member

mpeyper commented Apr 4, 2019

I'm stil looking at this but struggling a bit when the promise rejects (seems to go into a infinite spin somewhere and I'm not sure why).

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2019

oh ya, that only handles happy path. I'll update my comment to include rejection

@ntucker
Copy link
Contributor Author

ntucker commented Apr 4, 2019

in the real world the error would probably be set somewhere else, but this is just the dead simple handler

@mpeyper
Copy link
Member

mpeyper commented Apr 4, 2019

Thanks. In my head, if the thrown promise rejects, it would trigger the error boundary, but in practice, I see that this is not the case. Explicitly throwing the error does pass my test as expected.

I did have a variation of this that would capture the error from the thrown promise, which was nicer in that you wouldn't have to work out some way of capturing the error to throw yourself in the hook, but I'm thinking now that we want the Suspense handling to be as close to what React does as possible so that we don't introduce scenarios where it's easy to pass the test but they fail in the real world.

@ntucker
Copy link
Contributor Author

ntucker commented Apr 5, 2019

Yeah, the throw has to originate on the react render call stack. But async events have their own callstack. Adding the .catch() for the promise allows us to grab it from the async callback so we can then throw it in the render call stack.

It actually makes a lot of sense when you think about it that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
2 participants