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

Portal stays visible indefinitely if unmounted immediately after mounting #2606

Closed
matthieugicquel opened this issue Mar 15, 2021 · 11 comments
Closed
Labels

Comments

@matthieugicquel
Copy link

matthieugicquel commented Mar 15, 2021

Current behaviour

When trying to remove a portal immediately after mounting it, it stays visible indefinitely. See the failing test below.

Expected behaviour

The portal is unmounted and does not stay visible.

Code sample

Here's a minimal failing test case (something equivalent happened to me in more complicated, real world code)

import React, { useEffect, useState } from 'react';
import { Portal, Text } from 'react-native-paper';
import { render, waitForElement } from 'react-native-testing-library';

const TestComponent = () => {
  const [isPortalVisible, setIsPortalVisible] = useState(true);
  useEffect(() => {
    // Unmount the portal immediately after mounting it
    setIsPortalVisible(false);
  }, []);

  return (
    <Portal.Host>
      {isPortalVisible && (
        <Portal>
          <Text>{"I'm here"}</Text>
        </Portal>
      )}
    </Portal.Host>
  );
};

test('portal is properly unmounted', async () => {
  const component = render(<TestComponent />);

  // This expectation is failing, the portal is never unmounted, despite the useEffect
  await waitForElement(() => {
    const portalContent = component.queryByText("I'm here");
    if (portalContent) throw new Error('Portal is still visible');
  });
});

What have you tried

The cause seems to be with the await here:

async componentDidMount() {
this.checkManager();
// Delay updating to prevent React from going to infinite loop
await Promise.resolve();
this.key = this.props.manager.mount(this.props.children);
}

The await in componentDidMount causes a race condition with the synchronous unmounting:

componentWillUnmount() {
this.checkManager();
this.props.manager.unmount(this.key);
}

The portal gets (not really) unmounted before being mounted → it stays registered with the portal manager.

One possible fix is to make componentWillUnmount async too:

-  componentWillUnmount() {
+  async componentWillUnmount() {
     this._checkManager();

+    await Promise.resolve();
     this.props.manager.unmount(this._key);
   }

I'm not sure why the await Promise.resolve() was introduced, so I can't tell for sure if it's possible to fix this in a cleaner way.

Your Environment

software version
ios or android irrelevant
react-native 0.63.3
react-native-paper bug confirmed in 2.16 and 4.7.2
node 14.15
npm or yarn irrelevant
expo sdk not used
@github-actions
Copy link

Couldn't find version numbers for the following packages in the issue:

  • react-native
  • react-native-paper
  • react-native-vector-icons
  • npm
  • yarn
  • expo

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

@github-actions
Copy link

Couldn't find version numbers for the following packages in the issue:

  • react-native-paper
  • react-native-vector-icons
  • npm
  • yarn
  • expo

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

The versions mentioned in the issue for the following packages differ from the latest versions on npm:

  • react-native (found: 0.63.3, latest: 0.64.0)

Can you verify that the issue still exists after upgrading to the latest versions of these packages?

@juspeeh
Copy link

juspeeh commented Mar 25, 2021

Facing same issue. Any workarounds?

@AndresAltomobile
Copy link

Facing the same issue. I tried modifying PortalConsumer.tsx as pointed out by @matthieugicquel and it works in debug mode, but when I try in release mode it keeps happening. Any workaround for release/prod mode?

react-native-paper: 4.7.1
react-native: 0.63.4

@raajnadar
Copy link
Collaborator

use https://www.npmjs.com/package/patch-package

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

Hello 👋, this issue has been open for more than 2 months with no activity on it. If the issue is still present in the latest version, please leave a comment within 7 days to keep it open, otherwise it will be closed automatically. If you found a solution on workaround for the issue, please comment here for others to find. If this issue is critical for you, please consider sending a pull request to fix the issue.

@github-actions github-actions bot added the Stale label Jun 9, 2021
@fnimick
Copy link

fnimick commented Jun 16, 2021

Can confirm this is still an issue in the latest version. Our solution is to always render a Portal and have actual contents or an empty body based on our condition, rather than conditional rendering of the Portal itself.

@angelos3lex
Copy link

Any case this gets opened again and be fixed?

@OrLevy23
Copy link

why is this closed? this is still an issue

@ali-rantakari
Copy link

ali-rantakari commented Nov 26, 2021

This issue should be reopened. We've been hit by a similar race condition in componentDidUpdate, otherwise matching the description in this issue.

this.props.manager.update(this.key, this.props.children);

This old PR looks like it might fix this: #2226

@juspeeh
Copy link

juspeeh commented Dec 7, 2021

Still an issue

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

No branches or pull requests

8 participants