Portal example can cause issues if children access the DOM on componentDidMount

[darrenscerri]

In the Portal example, the portal container is mounted in the DOM in the Modal's componentDidMount method

componentDidMount() {
  modalRoot.appendChild(this.el);
}

This means that when the portal is being mounted, children are initially mounted on a detached node and their componentDidMount is called when the component's node is not yet inserted in the DOM. Therefore, any DOM method that requires nodes to be mounted (getting node dimensions, focusing an element, etc.) and props like autoFocus will not work.

class Child extends React.Component {
  componentDidMount() {
    setTimeout(() => {
      let node = ReactDOM.findDOMNode(this);

      console.log(node.clientWidth);
      // 1980

      console.log(document.body.contains(node));
      // true
    }, 0);

    let node = ReactDOM.findDOMNode(this);

    console.log(node.clientWidth);
    // 0

    console.log(document.body.contains(node));
    // false
  }
  // The click event on this button will bubble up to parent,
  // because there is no 'onClick' attribute defined
  render() {
    return (
      <div className="modal">
        {/* Input will not focus */}
        <input autoFocus />
        <button>Click</button>
      </div>
    );
  }
}

This can be fixed by either moving Modal's appendChild method to componentWillMount or using the pattern used by https://github.com/tajo/react-portal/blob/master/src/Portal.js:

class Modal extends React.Component {
  componentWillUnmount() {
    if (this.el) {
      modalRoot.removeChild(this.el);
    }
  }

  render() {
    if (!this.el) {
      this.el = document.createElement("div");
      modalRoot.appendChild(this.el);
    }
    return ReactDOM.createPortal(this.props.children, this.el);
  }
}

[bvaughn]

Hi @darrenscerri 😄

componentWillMount and render methods should not have side effects. Side effects (like modifying the DOM) are only safe in componentDidMount, componentDidUpdate, and componentWillUnmount methods. The docs mention this briefly but don't go into a lot of detail. Sebastian explains it in more detail on issue facebook/react/issues/7671

I don't think the example in question is that problematic, since the portal is a functional component and so can't have refs.

I'd be happy to review a PR that adds a note that mentions the concern you've pointed out though, if you'd like to submit one? 😄

[darrenscerri]

Hi @bvaughn :)

Agreed, that's a valid point. I guess the idiomatic solution would be something like:

class Modal extends React.Component {
  constructor(props) {
    super(props);
    this.el = document.createElement("div");
    this.state = {
      mounted: false
    };
  }
  componentDidMount() {
    modalRoot.appendChild(this.el);
    this.setState({
      mounted: true
    });
  }
  componentWillUnmount() {
    if (this.el) {
      modalRoot.removeChild(this.el);
    }
  }

  render() {
    return ReactDOM.createPortal(
      this.state.mounted ? this.props.children : null,
      this.el
    );
  }
}

This guarantees that any children's componentDidMount is called when that child is inserted in the DOM tree, and enabling props like autoFocus to work as expected :)

My concern is that users may tend to use the Modal example from the official docs as a starting point for their modals and may hit the mentioned issues later on, which in certain cases may not be so obvious to figure out what's going wrong.

Thoughts?

[bvaughn]

Hm, I think that's reasonable. Want to put together a PR? Maybe with an inline comment or two explaining why it does more than may be seemingly necessary at first glance? 😄

[darrenscerri]

Sure! Opened a PR here.

[Spenc84]

I apologize for resurfacing a long closed issue, but after thoroughly searching the rest of the web this still seems like the most relevant place to post my concern.

As @darrenscerri nicely pointed out, if you implement a portal using the example code from the Modal component in React's docs, any child rendered by that portal would not have access to the dom during it's own componentDidMount lifecycle method, which is problematic if a child needs to manipulate or measure the dom at that time. @darrenscerri suggested using state to prevent rendering the children until after the Modal component has had a chance to mount itself, thus allowing the children to initially render into an element that's already been injected into the dom. That solves the problem for the children, but creates a new problem for any ancestors of the Modal component that might need to access a ref to one of those children during their own componentDidMount lifecycle methods, since those children won't get mounted until all of their ancestors have finished mounting.

So it appears I'm left with a glaring issue that doesn't seem to have any good resolution. I either implement a portal component like this:

class Portal extends React.Component {
  constructor(props) {
    super(props);
    this.el = document.createElement('div');
  }

  componentDidMount() {
    portalRoot.appendChild(this.el);
  }

  componentWillUnmount() {
    portalRoot.removeChild(this.el);
  }

  render() {
    return ReactDOM.createPortal(
      this.props.children,
      this.el,
    );
  }
}

...which will limit what children can be used by the component since those children won't have access to the dom when they're initially mounted.

Or I implement a portal component like this:

class Portal extends React.Component {
  constructor(props) {
    super(props);
    this.el = document.createElement('div');
    this.state = {
        mounted: false
    };
  }

  componentDidMount() {
    portalRoot.appendChild(this.el);
    this.setState({ mounted: true });
  }

  componentWillUnmount() {
    portalRoot.removeChild(this.el);
  }

  render() {
    return this.state.mounted && ReactDOM.createPortal(
      this.props.children,
      this.el,
    );
  }
}

...which means you won't be able to access a reference to any child of this component during it's initial mount.

Or I implement a portal component like this:

class Portal extends React.Component {
  constructor(props) {
    super(props);
    this.el = document.createElement('div');
    portalRoot.appendChild(this.el);
  }

  componentWillUnmount() {
    portalRoot.removeChild(this.el);
  }

  render() {
    return ReactDOM.createPortal(
      this.props.children,
      this.el,
    );
  }
}

This will ensure that both the children and ancestors of this component will have access to what they need during their componentDidMount lifecycle methods, but it creates the possibility of stale nodes should the component ever get removed prior to finishing it's mounting cycle.

From what I can tell, there doesn't appear to be a good way to implement a Portal component without having to deal with one of these problems.

Am I missing something?