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

findDOMNode() throws inside Suspense #14188

Closed
will-hart opened this issue Nov 11, 2018 · 19 comments
Closed

findDOMNode() throws inside Suspense #14188

will-hart opened this issue Nov 11, 2018 · 19 comments

Comments

@will-hart
Copy link

@will-hart will-hart commented Nov 11, 2018

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

I have a component which listens for resize events (via a BlueprintJS ResizeSensor). When loading a component dynamically with lazy / Suspense, an exception occurs as the resize sensor appears to be unmounted:

Uncaught Error: Unable to find node on an unmounted component.
    at invariant (29.chunk.js:86295)
    at findCurrentFiberUsingSlowPath (29.chunk.js:90628)
    at findCurrentHostFiber (29.chunk.js:90640)
    at findHostInstanceWithWarning (29.chunk.js:106349)
    at findDOMNode (29.chunk.js:106869)
    at ResizeSensor.componentDidUpdate (29.chunk.js:10535)

The resize sensor should be removing listeners on unmount.

Demo: https://codesandbox.io/s/n4241q075l
Related: palantir/blueprint#3141

What is the expected behavior?

No exception is thrown.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Tested with:

  • React 16.6.1, fails
  • React 16.6.0, works
@will-hart will-hart changed the title Suspense Suspense and lazy component throws exception with unmounted children Nov 11, 2018
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Nov 11, 2018

Can you test if it works on master?

@arianon

This comment has been minimized.

Copy link

@arianon arianon commented Nov 11, 2018

Nope, it's broken on master, and after bisecting, I confirmed the regression was caused by #14083

EDIT: Something else I found is that before the regression, ResizeSensor.componentDidUpdate doesn't execute, but now it does.

EDIT 2: Yeah, ReactDOM.findDOMNode throws when invoked with a suspended component, here's how you can reproduce.

const React = require("react");
const ReactDOM = require("react-dom");
const ReactTestRenderer = require("react-test-renderer");
const { Suspense } = React;

let didSuspend = false;
function Suspend() {
  if (!didSuspend)
    throw {
      then() {
        didSuspend = true;
      }
    };
  return null;
}

class Component extends React.Component {
  componentDidUpdate() {
    ReactDOM.findDOMNode(this);
  }

  render() {
    return null;
  }
}

function App() {
  return (
    <Suspense fallback="Loading">
      <Component />
      <Suspend />
    </Suspense>
  );
}

const root = ReactTestRenderer.create(<App />);
root.update(<App />);

This is true both before and after the aforementioned PR, so in any case it looks like it helped us discover this 😄

@gaearon gaearon changed the title Suspense and lazy component throws exception with unmounted children findDOMNode() throws inside Suspense Nov 12, 2018
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Nov 12, 2018

EDIT: Something else I found is that before the regression, ResizeSensor.componentDidUpdate doesn't execute, but now it does.

This sounds like a separate problem, can you provide a test case for it?

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Nov 12, 2018

Here's a minimal repro with ReactDOM:

const Lazy = React.lazy(() => new Promise(resolve => resolve({
  default() {
    return 'hello';
  }
})))

class Child extends React.Component {
  componentDidUpdate() {
    ReactDOM.findDOMNode(this);
  }

  render() {
    return null;
  }
}

class App extends React.Component {
  state = {
    suspend: false
  };
  handleClick = () => {
    this.setState({ suspend: true });
  };
  render() {
    return (
      <React.Suspense fallback="Loading">
        <Child />
        <button onClick={this.handleClick}>Suspend</button>
        <Child />
        {this.state.suspend && <Lazy />}
      </React.Suspense>
    );
  }
}

ReactDOM.render(
  <App />,
  document.getElementById('container')
);
gaearon added a commit to gaearon/react that referenced this issue Nov 12, 2018
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Nov 12, 2018

Okay, so a part of this is covered by #14197. It got first broken by #14083 and got fixed in #14164.

However I think repro case in #14188 (comment) might be a separate issue. Looking into it.

@arianon

This comment has been minimized.

Copy link

@arianon arianon commented Nov 12, 2018

This sounds like a separate problem, can you provide a test case for it?

Welp, turns out I'm wrong, I actually made a mistake when mocking componentDidUpdate which resulted in it not being called 😝

gaearon added a commit that referenced this issue Nov 13, 2018
sophiebits added a commit to gaearon/react that referenced this issue Dec 6, 2018
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
n8schloss added a commit to n8schloss/react that referenced this issue Jan 31, 2019
iyegoroff added a commit to iyegoroff/react that referenced this issue Mar 9, 2019
@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Apr 9, 2019

Another reproducible example: https://codesandbox.io/s/zwzqp18ool

When does it throw:

  • 16.6.0: never
  • 16.6.1: on every "display lazy" click
  • 16.7.0 until 16.8.6: on every "display lazy" click if the number of "force update" clicks is odd (meaning even number of render?)
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Apr 9, 2019

This was likely fixed in #15312. Can you verify on master?

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Apr 9, 2019

This was likely fixed in #15312. Can you verify on master?

Yes, current master fixes #14188 (comment) for me.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Apr 9, 2019

This will get into the next release then.

@zombieJ

This comment has been minimized.

Copy link
Contributor

@zombieJ zombieJ commented Jun 10, 2019

@gaearon @acdlite,
Seems it still happens. I tried to get mini reproduce but it seems happened in complex environment.

User report reproduce: https://codesandbox.io/s/antd-reproduction-template-sbsiz
Quick click the nav item will throw with Unable to find node on an unmounted component.

ref: ant-design/ant-design#17016


Update:

Create a mini reproduce about this: #15857

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jun 19, 2019

Sorry, we didn’t release the fix yet.

@joshnabbott

This comment has been minimized.

Copy link

@joshnabbott joshnabbott commented Jul 1, 2019

@gaearon any ETA on when this will be available?

@ambroselittle

This comment has been minimized.

Copy link

@ambroselittle ambroselittle commented Jul 5, 2019

We are blocked by this, too (without a pretty significant workaround). Any sense of a release timeframe? @gaearon? Is there some way to find out (in general) when the "next" release is? Sorry if this is covered somewhere..

For the record, this is surfacing for us through Semantic UI React Dropdown. So we're beholden to them as well, but they point us here for the fix. :)

@wenisy

This comment has been minimized.

Copy link

@wenisy wenisy commented Jul 16, 2019

Sorry, we didn’t release the fix yet.

@gaearon

any new updates?

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jul 30, 2019

We've published 0.0.0-db3ae32b8 of all packages, which includes this fix. It is likely to be a release candidate for 16.9.0. You can give it a try now and see whether it resolves the issue for you. Thanks.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jul 30, 2019

(You should still ask packages to migrate away from findDOMNode though as it's eventually going to get deprecated.)

@MissLlq

This comment has been minimized.

Copy link

@MissLlq MissLlq commented Sep 3, 2019

We've published 0.0.0-db3ae32b8 of all packages, which includes this fix. It is likely to be a release candidate for 16.9.0. You can give it a try now and see whether it resolves the issue for you. Thanks.

16.9.0 hadn't fix this bug!

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Sep 3, 2019

We've published 0.0.0-db3ae32b8 of all packages, which includes this fix. It is likely to be a release candidate for 16.9.0. You can give it a try now and see whether it resolves the issue for you. Thanks.

16.9.0 hadn't fix this bug!

@MissLlq All reproductions linked in this issue (https://codesandbox.io/s/zwzqp18ool and https://codesandbox.io/s/n4241q075l) no longer crash with 16.9.

If you still encounter this error message with 16.9 please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.