Feature: add addCleanup util function

[huchenme]

What:

added addCleanup util function to be called after unmount happen in cleanup process

Why:

to add additional cleanups besides internal cleanups (unmount) and these callbacks could be called automatically in auto cleanup process

import { renderHook, addCleanup } from '@testing-library/react-hooks'

addCleanup(() => {
  console.log('addCleanup')
})

test('testing', () => {
  const hookWithCleanup = () => {
    useEffect(() => {
      return () => {
        console.log('cleanup')
      }
    })
  }
  renderHook(() => hookWithCleanup())
})

it will log cleanup and then addCleanup

How:

added addCleanup util function to add additional cleanups and to be called after internal cleanup happen

Checklist:

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

[codecov]

Codecov Report

Merging #432 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #432   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          107       109    +2     
  Branches        19        19           
=========================================
+ Hits           107       109    +2     

Impacted Files	Coverage Δ
src/pure.js	100.00% <ø> (ø)
src/cleanup.js	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 91734c5...8d0fc82. Read the comment docs.

[huchenme]

The reason I need to run something after unmount is that in my hook the cleanup create a timer to cleanup things of an instance of a Class which is deep in the dependency tree.

hook.js

import service from './service'
function myHook() {
  useEffect(() => {
    return () => {
      service.cleanup(id)
    }
  })
}

service.js

import TheService from './theservice'
//...
export default new TheService()

theservice.js

export default class TheService {
  constructor() {
    this.someList = new Set()
  }
  cleanup(id) {
    setTimeout(() => {
      this.someList.remove(id)
    }, 2000)
  }
}

so we will need to make sure this.someList.remove(id) get called after unmount between each test, otherwise this.someList will have wrong value between tests

[huchenme]

@mpeyper could you help review this PR? thanks a lot

[mpeyper]

I understand the issue you're trying to solve here, but I feel like the use-case is quite niche and I believe it might be possible to achieve by opting out of auto cleanup and handling it yourself for this scenario:

import { renderHook, cleanup } from '@testing-library/react-hooks/pure'

afterEach(async () => {
  await cleanup()
  console.log('afterUnmount')
})

test('testing', () => {
  const hookWithCleanup = () => {
    useEffect(() => {
      return () => {
        console.log('cleanup')
      }
    })
  }
  renderHook(() => hookWithCleanup())
})

I also feel like there are a few things in the implementation here that go a bit against my vision for this library. Firstly, there is an implicit, but no obvious connection between the afterMount function and unmounting the hook:

afterUnmount(() => {
  console.log('this got called')
})

test('example', () => {
  const { result, unmount } = renderHook(() => hookWithCleanup())

  // a bunch of test...

  unmount() // afterUnmount is called now

  // some more test...

  expect(result.current).toBe('something')
})

This is an example of something that makes perfect sense when you are writing the test, but bring someone else in or come back to this in a year's time and there are no threads to follow between when unmount is called and the callback executing. I'd much prefer something in the test itself:

test('example', () => {
  const { result, unmount, afterUnmount } = renderHook(() => hookWithCleanup())

  afterUnmount(() => {
    console.log('this got called')
  })

  // a bunch of test...

  unmount() // afterUnmount is called now

  // some more test...

  expect(result.current).toBe('something')
})

(yes, you can just move the call into the test, but having it as a root export makes the alternative possible)

Secondly, calling the function twice undoes the original:

afterUnmount(() => {
  console.log('this wont get called')
})

afterUnmount(() => {
  console.log('this got called')
})

test('example', () => {
  renderHook(() => hookWithCleanup())
})

This is unlikely to occur in a small test file, but with nested describe sections, imported utilities from other files or global setup file, there is a non-zero chance that this will create a hard to spot and hard to debug issue for someone. A setup more similar to the cleanup functionality, where cleanups are pushed into a list would feel much safer to me.

Speaking of cleanups, would exposing addCleanup also solve your issue. the unmount cleanup handles the acting to wait for effects and will always be the first one in the list, so adding another should have the same effect, no? Something like:

test('example', () => {
  const { addCleanup } = renderHook(() => hookWithCleanup())

  addCleanup(() => {
    console.log('this got called')
  })
})

or even

import { renderHook, addCleanup } from '@testing-library/react-hooks'

addCleanup(() => {
  console.log('this got called')
})

test('example', () => {
  renderHook(() => hookWithCleanup())
})

(It's more acceptable to me here in the global context because cleanup is implicitly global and not actually tied to unmount)

This has the caveat that it would not play after a manual unmount, but it is a more generic solution to the problem of running additional cleanup after the built in cleanup has occurred. We'd probably want to support async cleanup handlers if we did this.

To summarise, I don't think this utility is useful enough to enough people for me to take on the maintenance of it in this library. I might be wrong and I'm happy to hear alternative points of view.

[huchenme]

Hi @mpeyper, thanks for the detailed write-up and propose some other solutions.

I believe it might be possible to achieve by opting out of auto cleanup and handling it yourself for this scenario:

it will work, but it also means I will need to call manual unmount at the end of each test files, which is something I am trying to avoid in my test to remove some duplications

I also feel like there are a few things in the implementation here that go a bit against my vision for this library. Firstly, there is an implicit, but no obvious connection between the afterMount function and unmounting the hook:

for the case you provided, it could simply write

unmount()
console.log('after unmount')

so the afterUnmount API in this PR is more for auto cleanup or remove duplication of the same thing for each test

Secondly, calling the function twice undoes the original:

This is a really issue which I did not notice, which might require some additional work, if you think the overall proposal is good

const { addCleanup } = renderHook(() => hookWithCleanup())

if it is within a single test, people could just call cleanup manually before the end of the test, this API is not solving this issue

import { renderHook, addCleanup } from '@testing-library/react-hooks'

Currently addCleanup is not exposed in API, and it also won't work as intended: as the unmount cleanup is added in each test renderHook call (https://github.com/testing-library/react-hooks-testing-library/blob/master/src/pure.js#L92), and

addCleanup(() => {
  console.log('this got called')
})

is added globally first, the cleanup will be called before unmount happen, but what I want to achieve is to call something after unmount,

secondly, the cleanup added will be removed in each tests (https://github.com/testing-library/react-hooks-testing-library/blob/master/src/cleanup.js#L8), so this got called will only be printed for the first tests.

In summary, I want to continue using auto cleanup to unmount after each test, as there is no other way to unmount after tests without repeating (unless we have some APIs like below), but still run some callback after unmounting.

afterEach(() => {
  unmountTheHook()
  afterUnmount()
})

If you have better ideas, please let me know

[mpeyper]

I don't have a huge amount of time for a reply, so just a quick note for now

it will work, but it also means I will need to call manual unmount at the end of each test files, which is something I am trying to avoid in my test to remove some duplications

cleanup should unmount any minted components without the need to call unmount in each test. That's why it exists in the first place.

[huchenme]

cleanup should unmount any minted components without the need to call unmount in each test. That's why it exists in the first place

yes, I understand, but how to make sure some additional things run after the unmount happen in auto cleanup in each test?

[mpeyper]

I'm sorry @huchenme I don't quite follow. If you opt out of auto cleanup then you control when the cleanup happens and what code runs before or after it. You won't need to call unmount at the end of your test though because cleanup will unmount first. Can you show me an example of where this approach results in significantly more duplicated code or doesnt call the cleanup steps in the correct order?

import { renderHook, cleanup } from '@testing-library/react-hooks/pure'

afterEach(async () => {
  await cleanup()
  console.log('afterUnmount')
})

test('testing', () => {
  const hookWithCleanup = () => {
    useEffect(() => {
      return () => {
        console.log('cleanup')
      }
    })
  }
  renderHook(() => hookWithCleanup())
})

Currently addCleanup is not exposed in API, and it also won't work as intended...

hmm, yes. I see what you mean. Purely exposing addCleanup as it's currently written won't work.

Just spitballing the idea a little bit, what if the global addCleanup were (optionally?) persistent and always ran after cleanups that were registered during renderHook? Sort of like some kind of scoping mechanism when the renderHook pushes a new scope in and cleanup (or unmount?) pops it out again, leaving the global scope in place. Obviously this idea hasn't been fully thought through, but I think it has some legs.

This might be a bit of a bigger build than just what you require, so if you want to make an issue and reference this PR in it, I'm happy to explore it myself. By all means though, if you want to have a go and modify this PR along those lines, I'll gladly review it.

My issue really is with this your proposal as written is that unmount is a turned into a global level concern and the link from each test to the global handler is implicit. Simply calling it addCleanup breaks that mental link, even if it there under the hood, and leaves the door open to extend it to other forms of additional cleanup, not just for more precise timing around cleanup steps.

[huchenme]

Sorry I have not thought about this could achieve what I wanted

import { renderHook, cleanup } from '@testing-library/react-hooks/pure'

afterEach(async () => {
  await cleanup()
  console.log('afterUnmount')
})

But after reading your suggestions on addCleanup, I think it is also good and it is more general compare with afterUnmount, I decided to have a try implementing it. I have changed current PR, could you take a look and let me know what you think?

[mpeyper]

Sorry @huchenme, I am planning on coming back to this, I've just been busy the last few days and might not yet to it for another few.

[mpeyper]

Hi @huchenme,

I finally found enough time to give this a proper look. While I was reviewing, I struggled with the idea that the cleanups were going to be global and would persist between tests, if they were only registered in a specific describe scope or even in a specific test. It dawned on me that the the only real thing preventing addCleanup from working as written was that the unmount ran last and if we just run the cleanup handlers in reverse order, it would run before the externally registered ones.

After playing around with that idea for a while, it started to feel almost more correct that the last cleanup added should be the first one to run. This is similar to how things like beforeEach and afterEach generally work in test runners too.

The changes I pushed to your branch would make your example look something likes this:

import { renderHook, cleanup, addCleanup } from '@testing-library/react-hooks'

beforeEach(() => {
  addCleanup(() => console.log('afterUnmount'))
})

test('testing', () => {
  const hookWithCleanup = () => {
    useEffect(() => {
      return () => {
        console.log('cleanup')
      }
    })
  }
  renderHook(() => hookWithCleanup())
})

How does that look to you? Workable?

I also added a few niceties like a removeCleanup utility (as well as addCleanup returning a pre-bound remove function) and the ability handle asynchronous cleanups.

I'm still on the fence to how much this functionality will be used, but let me know if you are ok with my changes as I feel like there is very little overhead in maintaining this version (it's basically the same functionality we used internally), so if you can find some value in it, other might too.

[huchenme]

Hi @mpeyper thanks for the changes, I think it works for my case!

[mpeyper]

I've thought long and hard about this, and I've decided to merge it. I've got a few other changes I want to make so it might not make a release immediately, but it'll come along in the next version.

[github-actions]

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀