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

Unmount doesn't seem to be firing useEffect cleanup functions #847

Open
kmarple1 opened this issue May 25, 2022 · 15 comments
Open

Unmount doesn't seem to be firing useEffect cleanup functions #847

kmarple1 opened this issue May 25, 2022 · 15 comments
Labels
bug Something isn't working

Comments

@kmarple1
Copy link

kmarple1 commented May 25, 2022

  • react-hooks-testing-library version: 8.0.0
  • react version: ^17.0.2
  • react-dom version (if applicable): ^17.0.2
  • react-test-renderer version (if applicable): n/a
  • node version: v14.17.6
  • npm (or yarn) version: 6.14.15

Relevant code or config:

The component I'm testing:

import { useState, useEffect } from "react";

const useDebounce = (value, delay) => {
    const [debouncedValue, setDebouncedValue] = useState(value);

    useEffect(() => {
        const handler = setTimeout(() => {
            setDebouncedValue(value);
        }, delay);

        return () => {
            clearTimeout(handler);
        };
    }, [value]);

    return debouncedValue;
};

export default useDebounce;

The test itself:

import { renderHook } from "@testing-library/react-hooks";
import useDebounce from "../useDebounce";

jest.useFakeTimers();
jest.spyOn(global, "clearTimeout");

describe("useDebounce", () => {
    test(`it should clear the timeout when unmounting`, () => {
        const value = "test";
        const delay = 1000;
        const { unmount } = renderHook(() => useDebounce(value, delay));
        expect(clearTimeout).not.toHaveBeenCalled();
        unmount();
        expect(clearTimeout).toHaveBeenCalled(); // fails here
    });
});

What you did:

I'm attempting to test that the useEffect cleanup function in my hook fires correctly.

What happened:

While it works properly when running normally, I can't get unmount() to fire it.

Reproduction:

The above example reproduces the problem.

Problem description:

Unmount doesn't seem to be properly cleanup up useEffects.

@kmarple1 kmarple1 added the bug Something isn't working label May 25, 2022
@mpeyper
Copy link
Member

mpeyper commented May 27, 2022

I imagine it’s the fake timers getting in the way. Try without that line and see if it works (I’m unable to test it myself at the moment)

@AndyOGo
Copy link
Contributor

AndyOGo commented Sep 9, 2022

I have the same problem without any fake timers.

I have an RxJS observable which I unsubscribe in the effect cleanup.
Works in the browser, but not when running tests.

@dahliacreative
Copy link

I'm also having this issue, I call a function on the window in my effect cleanup but I can't asset that it's being called as the clean up function never get's called when unmounting.

Is anyone looking into this? Thanks.

@mpeyper
Copy link
Member

mpeyper commented Oct 19, 2022

Hi @dahliacreative,

Noone is currently looking into this but we have tests that ensure the effects are cleaned up, so it's unlikely to be an issue with this library or react itself (I cannot stress how much of our functionality is just calling react to do it's thing) and much more likely to be in your code. A common issue I see is that the work in the cleanup is actually async and has not run in between the unmount call and assertions happening.

If you want help diagnosing the issue, I suggest sharing some code here/a new "Question" issue/discord, or putting together a small reproduction repo if you cannot share your actual code with us.

@harshabikkavilli
Copy link

harshabikkavilli commented Nov 17, 2022

Have same issue - not firing useEffect cleanup on unmount. Here is the code snippet I am using:

Video.tsx
---------------
React.useEffect(() => {
    video.addEventListener('mouseover', onPlay);
    video.addEventListener('mouseout', onPause);
    return () => {
      video.removeEventListener('mouseover', onPlay);
      video.removeEventListener('mouseout', onPause);
    };
  }, []);
Video.spec.tsx
----------------
it('should call addEventListener on mount', () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();
    expect(removeEventListener).toHaveBeenCalled();
});

Tried wrapping unmount around act, but unmount already does that - so that's not it.
Any help here is appreciated.


Edit:
Okay, this worked for me:

Caching a reference to the useEffect's cleanup function

let cleanupFunc;
jest.spyOn(React, 'useEffect').mockImplementation((f) => {
    cleanupFunc = f();
});

Then finally calling this reference on unmount

it('should call addEventListener on mount', () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();
    cleanupFunc();
    expect(removeEventListener).toHaveBeenCalled();
});

@vzaidman
Copy link

try skiping one thread cycle before expecting:

it('should call addEventListener on mount', async () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();

    // wait for a moment
    await new Promise((resolve) => setTimeout(resolve, 1));

    expect(removeEventListener).toHaveBeenCalled();
});    

@ignatospadov
Copy link

This did not work for me @vzaidman but what did work @kmarple1 was the following:

Component

import { useEffect } from 'react'

const Component = () => {
  useEffect(async () => {
    console.log('renders')
    return () => {
      console.log('cleanup')
    }
  }, [url])

  return <div/>
}

export default Component
  it('unmounts', async () => {
    console.log = jest.fn()
    let cleanupFuncPromise
    jest.spyOn(React, 'useEffect').mockImplementationOnce(func => cleanupFuncPromise = func())
    const subject = await render(<Component/>)
    await subject.unmount()
    const cleanUpFunction = await cleanupFuncPromise
    cleanUpFunction()
    expect(console.log).toHaveBeenCalledWith('cleanup)
  })

@Fawwad-Khan
Copy link

Any updates on this?
I don't want to mock my useEffect as my useEffect is actually making changes in my redux store, and That would turn into alot of work, Im mocking each step just for this.

@mpeyper
Copy link
Member

mpeyper commented May 29, 2023

Hi @Fawwad-Khan No update from my end. If you want to share some code (or ideally a reproduction repo) I'm may be able to spot something obvious, but I don't have a lot of time for deep debugging on issues like this (I'll reiterate that it's unlikely to be this library causing the issue here).

Also, @ignatospadov, I've just noticed that your usage of useEffect here does not look valid to me. You cannot pass a async function to useEffect like that as far as I'm aware. To be valid it should looks more like:

const Component = () => {
  useEffect(() => {
    const run = async () => {
      console.log('renders')
    }

    run()

    return () => {
      console.log('cleanup')
    }
  }, [url])

  return <div/>
}

Hope that helps.

@Fawwad-Khan
Copy link

I cant really share the code as the organization I work at doesnt let me.
This is what it pretty much looks like.

Im unsetting the UIState on unmount but im still getting the "something" value after unmount in my test.
It works fine in the UI/Implementation.

const MyHook = () => {

const dispatch = useDispatch()
const {UIState} = useSelector(state => ({ UIState: undefined }))

  useEffect(() => {
   dispatch(setUIState("something"))

    return () => {
     dispatch(setUIState(undefined))
    }
  }, [dispatch, setUIState])

  return {
  UIState
  }
}

@saramorillon
Copy link

Hello, if this can be of any help, here is a reproduction code:

// src/useMount.ts

import { useEffect, useState } from 'react'

export function useMount() {
  const [mounted, setMounted] = useState(false)

  useEffect(() => {
    setMounted(true)
    return () => {
      setMounted(false)
    }
  }, [])

  return mounted
}
// tests/unit/useMount.test.ts

import { renderHook } from '@testing-library/react'
import { useMount } from '../../src/useMount'

describe('useMount', () => {
  it('should be mounted', () => {
    const { result } = renderHook(useMount)
    expect(result.current).toBe(true)
  })

  it('should not be mounted after unmount', () => {
    const { result, unmount } = renderHook(useMount)
    unmount()
    expect(result.current).toBe(false)
  })
})

Result:
image

Dependencies:

    "@testing-library/dom": "^9.3.1",
    "@testing-library/react": "^14.0.0",
    "@types/jest": "^29.5.2",
    "@types/react": "^18.2.13",
    "jest": "^29.5.0",
    "jest-environment-jsdom": "^29.5.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "ts-jest": "^29.1.0",
    "typescript": "^5.1.3"

This code is hosted here: https://github.com/saramorillon/hooks/tree/reproduction-847

@bismarkhenao
Copy link

bismarkhenao commented Feb 20, 2024

Hello, if this can be of any help, here is a reproduction code:

// src/useMount.ts

import { useEffect, useState } from 'react'

export function useMount() {
  const [mounted, setMounted] = useState(false)

  useEffect(() => {
    setMounted(true)
    return () => {
      setMounted(false)
    }
  }, [])

  return mounted
}
// tests/unit/useMount.test.ts

import { renderHook } from '@testing-library/react'
import { useMount } from '../../src/useMount'

describe('useMount', () => {
  it('should be mounted', () => {
    const { result } = renderHook(useMount)
    expect(result.current).toBe(true)
  })

  it('should not be mounted after unmount', () => {
    const { result, unmount } = renderHook(useMount)
    unmount()
    expect(result.current).toBe(false)
  })
})

Result: image

Dependencies:

    "@testing-library/dom": "^9.3.1",
    "@testing-library/react": "^14.0.0",
    "@types/jest": "^29.5.2",
    "@types/react": "^18.2.13",
    "jest": "^29.5.0",
    "jest-environment-jsdom": "^29.5.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "ts-jest": "^29.1.0",
    "typescript": "^5.1.3"

This code is hosted here: https://github.com/saramorillon/hooks/tree/reproduction-847

Here is how this hook is implemented on the usehooks-ts library. I hope this example can provide some helpful insights! 👍

@bhushanlaware
Copy link

Anyone found the solution, here is my code where issue is happening.

  React.useEffect(() => {
    emitter.on(MY_EVENT,callback);

    return () => { 
      console.log('unsubscribing'); // <<--- Its not getting console logged.
      emitter.off(MY_EVENT, callback);
    };
  }, []);
  it.only('should add and remove event listeners', () => {
    const { unmount } = render(<Component {...defaultProps} />);
    expect(emitter.on).toHaveBeenCalledTimes(1);
    expect(observers[MY_EVENT).toBeDefined();


    unmount();
    expect(emitter.off).toHaveBeenCalledTimes(1);
    expect(observers[MY_EVENT]).toBeUndefined();
  });
});
    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      23 |
      24 |     unmount();
    > 25 |     expect(emitter.off).toHaveBeenCalledTimes(1);
          |                         ^
      26 |     expect(observers[MY_EVENT]).toBeUndefined();

@mpeyper
Copy link
Member

mpeyper commented Apr 2, 2024

@bhushanlaware it looks like you are rendering with react-testing-library, not react-hooks-testing-library so I suggest raising an issue with them for your case.

As a general reminder to others coming into this issue, if you are importing renderHook from @testing-library/react and not @testing-library/react-hooks then you should also be raising an issue with react-testing-library instead of tacking on a +1 to this issue.

Also a general suspicion that fake timers, unhandled async or mocked useEffect implementations are the likely cause here rather than the testing libraries involved. We generally defer the actual rendering to react itself, so if effects are not being cleanup up, a lot more people would be complaining about it.

@patomation
Copy link

Would it help to use act?

act(() => unmount());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests