Skip to content

Return value of hook run with testHook? #295

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

Closed
danielkcz opened this issue Feb 12, 2019 · 7 comments · Fixed by #297
Closed

Return value of hook run with testHook? #295

danielkcz opened this issue Feb 12, 2019 · 7 comments · Fixed by #297
Labels

Comments

@danielkcz
Copy link
Contributor

Honestly, it got me confused big time. Considering that most of the tests might actually want to validate the output of the custom hook I find it strange we need to do this.

  test('accepts a default initial value for `count`', () => {
    let count
    testHook(() => ({count} = useCounter({})))

    expect(count).toBe(0)
  })

So I would like to propose getting the output directly.

  test('accepts a default initial value for `count`', () => {
    const { result } = testHook(() => useCounter({}))

    expect(result.count).toBe(0)
  })

It can be compared to a render where we are getting container (and bunch of getters) to write our expectations. The testHook is kinda inconsistent in this.

@alexkrolick
Copy link
Collaborator

See #274 (comment) for why this doesn't work - it's due to closures

@danielkcz
Copy link
Contributor Author

danielkcz commented Feb 12, 2019

Ok, in that case, we can do what I already mentioned somewhere else - return a getter.

  test('accepts a default initial value for `count`', () => {
    const { getResult } = testHook(() => useCounter({}))
    const { count } = getResult()
    expect(count).toBe(0)
  })

Alternatively, the useRef approach with a result.current, but personally, I would prefer a getter.

@danielkcz
Copy link
Contributor Author

danielkcz commented Feb 12, 2019

Ok, I have prepared a proof of concept that both approaches work.

https://codesandbox.io/s/209nj999jy?module=%2Fsrc%2FtestHook.test.js&previewwindow=tests

test("using a getter", () => {
  const { getResult } = testHook(() => useCounter({ step: 2 }));
  let { count, increment } = getResult();
  expect(count).toBe(0);
  act(() => {
    increment();
  });
  const { count: count2 } = getResult();
  expect(count2).toBe(2);
});
test("using a result.current", () => {
  const { result } = testHook(() => useCounter({ step: 2 }));
  expect(result.current.count).toBe(0);
  act(() => {
    result.current.increment();
  });
  expect(result.current.count).toBe(2);
});

Looking at it now I might move even more inclined to result.current considering naming clashes when destructuring from the getter.

@kentcdodds
Copy link
Member

I'm really struggling to understand how this is better than the current solution/example...

@danielkcz
Copy link
Contributor Author

danielkcz commented Feb 12, 2019

  • It's much easier with TypeScript, because let without an initial value has no clue about the type that will come later (maybe)
  • Imo it feels more natural to work with return value instead of hacking it inside the callback
  • It's closer to API of render
  • It would be optional, anyone can still do it the current way.

@kentcdodds
Copy link
Member

Ah, I see, that makes sense. Couldn't you do:

  test('accepts a default initial value for `count`', () => {
    const ref = {current: null}
    testHook(() => (ref.current = useCounter({})))

    expect(count).toBe(0)
  })

I guess I can see how that could be annoying. I'd be fine with returning a ref so long as the existing solution still works.

@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 5.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants