findDOMNode() throws inside Suspense

[will-hart]

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

[gaearon]

Can you test if it works on master?

[arianon]

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]

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]

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]

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 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 😝