Add render count

[Belco90]

What:
Implementing feature suggested to get how many times the component has been rendered #135

Why:
At the moment there is no way of knowing how many times a component has been rendered due to a hook update, and it's an interesting value for checking performance, memoized behaviors or forced updates.

How:
A count init to 0 within resultContainer that is increased every time updateResult is called, no matter if there is an error executing the hook as another render has been triggered so the counter must be increased to get corresponding count.

Checklist:

• Documentation updated
• Tests
• Typescript definitions updated
• Ready to be merged
  
• Added myself to contributors table
  

[codecov]

Codecov Report

Merging #136 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #136   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          39     42    +3     
  Branches        3      3           
=====================================
+ Hits           39     42    +3

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	⬆️

---

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 d3bc6c9...8eafb16. Read the comment docs.

[mpeyper]

Hi @Belco90,

Thanks for taking the time to put this together and the associated issue (#135).

I'm in two minds about this feature. My one half is thinking that this is a simple enough feature and to accept this, but my other half is worried that it's not quite in line with the spirit of the @testing-library packages. Let me explain...

I think the benefits of it is a bit more limited than you "Why" section describes with respect to testing for memoization. A hook will rerender the component on any state changes, at which point it can use memoised values to optimise its own performance, but the render still occurs. Unless you are testing the identity of some returned values to ensure they haven't changed, the test itself would be oblivious to whether memoization was used.

So this leaves us with testing performance and forced updates.

I'll tackle forced updates first as it's a bit easier to explain. Basically, it is very rare that you actually want to force an update in react. Generally speaking, the changing state of a component should be what triggers any updates and needing to force an update is usually a sign of something else not behaving correctly in the app. That said, we already provide the rerender function that will rerender the component and allow the hook's result to be recalculated if required.

The performance point is where the discussion gets interesting for me. On the surface, I'd say that testing for performance is a good thing. I'm just not sure using render counts is the best way to do this. I feel that a change like this will encourage users to focus on ensuring the component renders once, and only once, on any given action and spend a lot of time on micro-optimisations when, in reality, just letting react rerender the component is often good enough for the majority of use cases.

Essentially, it feels a little bit like we're getting bogged down in the details of how the result was calculated, rather than focussing on the correctness of the result itself. It would be easy to validly refactor the hook to render more or fewer times but still produce the same result, causing the test to now fail due to an implementation detail.

So I guess my objection is coming from a desire to help the users not think about any of this at the expense of a small number of valid use cases. In those cases, it is possible to provide render counts without changing this library, e.g:

  function useUpdate() {
    const [, setState] = useState(0)
    return () => setState((count) => count + 1)
  }

  test('should increase render count every time hook update function is called', () => {
    let renderCount = 0
    const { result } = renderHook(() => {
      renderCount++
      return useUpdate()
    })
    const update = result.current

    expect(renderCount).toBe(1)

    act(() => update())
    expect(renderCount).toBe(2)

    act(() => update())
    expect(renderCount).toBe(3)
  })

If you have lots of tests you want need to use this for a wrapper around renderHook can make it a bit more generic. Something like:

function renderHookWithCount(hookCallback) {
  const container = renderHook((...props) => {
    if (container) {
      container.result.renderCount++;
    }
    return hookCallback(...props);
  });
  container.result.renderCount = 1;
  return container;
}

  test('should increase render count every time hook update function is called', () => {
    const { result } = renderHookWithCount(() => useUpdate())
    const update = result.current

    expect(result.renderCount).toBe(1)

    act(() => update())
    expect(result.renderCount).toBe(2)

    act(() => update())
    expect(result.renderCount).toBe(3)
  })

All this being said, I'll think about this a bit more and listen to any feedback you or anyone else wants to share here about my opinions or other possible use cases this feature might enable and make a decision in the next day or two.

One thing to think about in the meantime is what should the renderCount be for a component that suspends in it's first render and completes in its seconds render? Again I'm torn on this. Part of me says it's obviously 2 renders , but the other part says it's 1 completed render. Right now I think your implementation will have renderCount as 1 in this scenario because we don't update the result when a promise is thrown.

[Belco90]

Aaah of course! Didn't think about increasing the count inside renderHook callback.

About the "why" of this feature implementation: you are right, it doesn't seem like following patterns from @testing-library and at the end the only interesting use case is performance.

Didn't think about the render count for suspend, I guess that makes things more complicated as my original proposal.

This is quite interesting:

Essentially, it feels a little bit like we're getting bogged down in the details of how the result was calculated, rather than focussing on the correctness of the result itself. It would be easy to validly refactor the hook to render more or fewer times but still produce the same result, causing the test to now fail due to an implementation detail.

So probably for performance this feature only makes sense as a profiling/measurement rather than a value for testing itself. But it looks like is the first time this has been mentioned (and I'm little bit surprised!), so probably all this is not necessary at all.

Your workaround solves my initial problem, but I'm happy to help implementing or discussing whatever is needed if you find this feature interesting for performance measurement.

[mpeyper]

I'm going to close this without merging for now. I'm still of the opinion that this is not needed and a variation of the wrapper above will work for most use cases for now. If there is a big demand in the future, we can revisit this then.

[sultan99]

Another way to count renders:

const renderHookWithCount = hook => renderHook(() => {
  const countRef = useRef(0)
  countRef.current ++
  return {renderCount: countRef.current, ...hook()}
})

const {result} = renderHookWithCount(() => ({
 someValue: someCustomHook()
})

expect(result.current.someValue).toBe(`some-value`)
expect(result.current.renderCount).toBe(1)

I have end up with this version:

const renderHookWithCount = hook => {
  let count = 0
  const renderCount = () => count
  const result = renderHook(() => {
    count ++
    return hook()
  })
  return {renderCount, ...result}
}

const {result, renderCount} = renderHookWithCount(someCustomHook)

expect(result.current).toBe(`some-value`)
expect(renderCount()).toBe(1)