Add a check in usePopper to stop setting state if hook has been unmounted

[MattyBalaam]

As mentioned in this issue https://github.com/popperjs/react-popper/issues/368 React Popper can throw act warnings in test which are due to calling a state setting function after the hook has been unmounted.

This adds a check to bail out of the updateStateModifier.fn before setting state.

[FezVrasta]

Thanks for the PR! Could you update the tests to get rid of the extra act calls to show the PR is working as intended?

[MattyBalaam]

Are you able to flag which act calls in the tests are for this purpose?

On Thu, 17 Dec 2020, 11:31 Federico Zivolo, notifications@github.com wrote:

Thanks for the PR! Could you update the tests to get rid of the extra act calls to show the PR is working as intended?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/popperjs/react-popper/pull/400#issuecomment-747383896, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJQNS6OBOSPLJAE5WQSK6LSVHTZBANCNFSM4U7PENSQ .

[FezVrasta]

This monster here is the main one I think: https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L10-L24

Also those waitFor: https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L64

[MattyBalaam]

Thanks, I'll check it out over the next few days.

On Thu, 17 Dec 2020 at 11:46, Federico Zivolo notifications@github.com wrote:

This monster here is the main one I think.

https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L10-L24

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/popperjs/react-popper/pull/400#issuecomment-747390852, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJQNS57C7BL6D3P2V2F6S3SVHVRLANCNFSM4U7PENSQ .

-- Matthew Balaam

---

development and design

matthewbalaam.co.uk http://www.matthewbalaam.co.uk @matthewbalaam https://twitter.com/matthewbalaam 07730 953486

[nsaunders]

@MattyBalaam thanks for persevering with fixing the tests. @FezVrasta I am looking forward to this being merged so my team will believe in this awesome library again. 🙏

[FezVrasta]

I'm looking forward too as soon tests are updated.

[mkrds]

@FezVrasta Hey, all that's left to be done in this PR is to remove act in https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L10-L24 and all the waitFors in https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L64 ?

[FezVrasta]

Correct!

[MattyBalaam]

Looks like I’m getting guilt tripped into finishing this up! I recall that I was stlll getting some act warnings and I needed to investigate further. I have some time today so I’ll see what I can do.

[MattyBalaam]

OK, I’m still looking into this, but currently have a theory it may be the debounce used in core library for the update function that could be the cause of these act warnings

[mkrds]

@MattyBalaam Yeah I can confirm that, I removed all the stuff from the Popper.test.js mentioned in the post above locally and unfortunately still got act warnings :/

[FezVrasta]

OK, I’m still looking into this, but currently have a theory it may be the debounce used in core library for the update function that could be the cause of these act warnings

The debounced function is forceUpdate, but it's a no-op after the Popper instance is destroyed, so it shouldn't be an issue.

[MattyBalaam]

OK, so I am currently thinking there are a couple of causes of act warnings at the moment in the library. I could be wrong, but these are

1. Calls to setState after unMounting - this is often an issue in tests other people will have written on their own components using react-popper.
2. The debounced initial update method call - the testing tools are unable to work out how to wait on their own and to get around these act warning you will normally either need to await some activity that indicates this update has happened, or wrap inside an act()

If you replace the update method in core @popper to be an undebounced function the 2nd set of act warnings disappear.

[MattyBalaam]

It’s been a bit of a frustrating all-in-all, so nothing to push as yet, but regarding that point two, the line in popper I’m referring to is:

https://github.com/popperjs/popper-core/blob/24cb3a88d5108d2a42126c8ab3899ee95259ef18/src/createPopper.js#L237

Unwrapping the call to debounce will lose a lot of act warnings, but obviously is not helpful for any kind of workable solution.

[FezVrasta]

Is it possible to check if the instance is still alive before trying to update the state? That should fix the issue.

[MattyBalaam]

@FezVrasta in this in relation to point one or point two?

When you say 'instance' is alive, do you mean to check for popperInstanceRef.current ? If so I think that may be able to used instead of the mount hook I have added. It would not have any effect on point two.

[FezVrasta]

What I meant is that we could check if the popper instance has been destroyed (by calling .destroy() on it) already or not.

[MattyBalaam]

Apologies, but that’s only confusing me more I’m afraid.

[piecyk]

This monster here is the main one I think: https://github.com/popperjs/react-popper/blob/master/src/Popper.test.js#L10-L24

IMHO we can remove this already as it's not needed, render from @testing-library is already wrapped with act. Please check quick poc https://github.com/popperjs/react-popper/compare/master...piecyk:poc-test-is-mounted

Still in test we need to wait for component to be rendered or it will throw, current practice is wait for something actually render.

[piecyk]

What I meant is that we could check if the popper instance has been destroyed (by calling .destroy() on it) already or not.

@FezVrasta is right, we don't need the useIsMounted as we can check if we have instance via popperInstanceRef.current before calling setState.

But really don't seeing a reason to have this check as react will remove elements so updateState modifier will not be called.

[MattyBalaam]

But really don't seeing a reason to have this check as react will remove elements so updateState modifier will not be called.

Unfortunately I don’t think this is the case, as functions inside function components can run after the component has been unmounted, hence the need for a check in this kind of situation. This is defintely the case with async functions in my experience, and I think this is what is happening here, but happy to be proved wrong.

[piecyk]

Unfortunately I don’t think this is the case, as functions inside function components can run after the component has been unmounted, hence the need for a check in this kind of situation. This is defintely the case with async functions in my experience, and I think this is what is happening here, but happy to be proved wrong.

Yes i agree that function components can run after the component has been unmounted, just don't seeing it here. When checking checkIsMounted() or better popperInstanceRef.current in tests, it will not call setState after unmounted.

Basic we would need to add a test that when unmounting component will trigger popperjs to run modifiers.

[MattyBalaam]

@piecyk sorry it seems like we were talking about cross-purposes, I had followed @FezVrasta’s advice and changed to a check for the instance locally, but I should have paid more attention reading your message (or not look at messages on a Friday night!)

It turns out I was almost there, and the last issue with the debounced update causing act warnings can be fixed by your suggestion of waiting for the assertions. ~Unforunately when using rerender in the hooks test we still need to wrap with act as far as I can see~ . But overall as far as I can see all tests now pass without act warnings.

I have taken a slightly different approach to your POC @piecyk and I have added a testid to await for the snapshot test, and in additon I felt it helped readability to move the snapshot inline.

There is already a test in the usePopper.test.js file for unmounting, but I can look at adding another test for unmounting the component in Popper.test.js if people think that would be a good addition.

[piecyk]

There is already a test in the usePopper.test.js file for unmounting, but I can look at adding another test for unmounting the component in Popper.test.js if people think that would be a good addition.

Yes, but my comment was about adding a test that would show the need for the popperInstanceRef.current check. IMHO if you remove this line, tests will still pass without throwing the act warning. If that is true do we want to add this line?

[MattyBalaam]

I was wondering how this could be tested... Perhaps spying on calls to console.warn and checking for a part of a string, but then it will be a little fragile if the warning changes

[piecyk]

I was wondering how this could be tested...

I think we can't.

Perhaps spying on calls to console.warn and checking for a part of a string, but then it will be a little fragile if the warning changes

That is correct approach here, but not to check string but if the methods was called. For example with this test we are checking that setState was not called for unmount component

it('should not perform a state update on an unmounted component', async () => {
  jest.spyOn(console, 'error').mockImplementation(() => {});

  const referenceElement = document.createElement('div');
  let forceUpdateRef;

  const { unmount } = render(
    <Popper referenceElement={referenceElement}>
      {({ ref, style, placement, forceUpdate }) => {
        forceUpdateRef = forceUpdate;

        return <div ref={ref} style={style} data-placement={placement} />;
      }}
    </Popper>
  );

  await waitFor(() => {
    unmount();
    forceUpdateRef();
  });

  expect(console.error).toHaveBeenCalledTimes(0);
  console.error.mockRestore();
});

As mentioned in this issue #368 React Popper can throw act warnings in test which are due to calling a state setting function after the hook has been unmounted.

IMHO this statement is wrong, react will log an error with warning when we would try to update state after the hook has been unmounted Warning: Can't perform a React state update on an unmounted component. This is a no-op... Not a case here as on unmount we call destroy from popperjs that will prevent calling setState.

The "not wrapped in act(...)" warning is not connected with this, basic it's bit confusing part of writing tests.

Adding instant check will not fix those warnings as that is not the main problem here.

Main problem are tests, let's improve them in this PR 💪

[anilanar]

This is a popperjs bug. Popperjs uses an internal debounce function that is not cancellable. When destroyed, debounced callbacks stay around until the debounce callback triggers (which is the next promise micro tick).

So, popperjs triggers a debounced forceUpdate which triggers the modifier fn, even after a destroy.

@FezVrasta fyi.

[FezVrasta]

@anilanar that makes sense, I'd be happy to accept a PR to cancel the update on destroy

[MattyBalaam]

I suspected the same thing here too. @FezVrasta you commented that this should not be a real-world issue because it runs as a no-op, although in tests we do get this act warning.

[piecyk]

So, popperjs triggers a debounced forceUpdate which triggers the modifier fn, even after a destroy.

Hmm @anilanar looking on popperjs code, isDestroyed https://github.com/popperjs/popper-core/blob/master/src/createPopper.js#L166-L168 prevents this 🤔

[davedx]

Hi @MattyBalaam and @FezVrasta, what's the status of this PR? I ask because in our project we use popper and have quite some warnings when we run the tests, and I can't seem to fix them in the tests themselves by wrapping in act(). I'm open to helping out too either by coding or testing, would be great to get this issue fixed. :)

[m7kvqbe1]

Is this dead? This issue is a nightmare.

[DanielGibbsNZ]

I'm still getting this issue with various components when using Mantine which uses react-popper under the hood.

I've worked around this in my test files by using a utility that runs cleanup immediately after the test, but this is not ideal:

import { cleanup } from '@testing-library/react`

const testWithCleanup = (test) => () => {
  test()
  cleanup()
}

it('test description here', testWithCleanup(() => { /* ... */ }))

This solution comes from the same issue raised in @testing-library/react: https://github.com/testing-library/react-testing-library/issues/999.

[atomiks]

This PR doesn't fix the issue IIRC. See: https://floating-ui.com/docs/react-dom#testing

Your wrapper is the workaround for now, unless RTL can fix it internally

[MattyBalaam]

Now that popper has been superceeded by floating-ui, I'm going to close this. Long term testing showed this did not actually avoid the warning anyway.

[m7kvqbe1]

It's also an issue in the new floating-ui implementation.

[MattyBalaam]

Unfortunately currently there is nothing that can be done for that except to follow the advice and make sure you manually leave tests in a state they will not fire state updates: https://github.com/floating-ui/floating-ui/issues/1520

https://floating-ui.com/docs/react-dom#testing

[atomiks]

It might be worth making an issue for @testing-library/react to see if their auto-cleanup unmounting behavior can fire before microtasks run, then the issue will be gone.

[atomiks]

fwiw I made an issue if you want to follow it https://github.com/testing-library/react-testing-library/issues/1073