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

componentDidUpdate fires before ref is set in React 16 #12034

Closed
mnpenner opened this issue Jan 17, 2018 · 6 comments
Closed

componentDidUpdate fires before ref is set in React 16 #12034

mnpenner opened this issue Jan 17, 2018 · 6 comments

Comments

@mnpenner
Copy link
Contributor

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

Bug

What is the current behavior?

componentDidUpdate fires before setRef

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://codesandbox.io/s/vnmw1090ly

Open the Console to see what's going on:

image

Pasting code here too for safe-keeping:

import React from 'react';
import ReactDOM, {render} from 'react-dom';
import Hello from './Hello';

const styles = {
    fontFamily: 'sans-serif',
    textAlign: 'center',
};

const App = () => (
    <div style={styles}>
        <h1>componentDidUpdate vs ref timing issue</h1>
        <ComboBox/>
    </div>
);

class ComboBox extends React.Component {

    setRef = node => {
        console.log('set ref');
        this.div = node;
    }

    constructor(props) {
        super(props);
        this.state = {open: false};
        setTimeout(() => {
            this.setState({open: true}); // force an update for the purposes of this example
        }, 500);
    }


    componentDidUpdate() {
        console.log('hello', this.div);
    }

    render() {

        if(!this.state.open) {
            return <div>closed</div>
        }

        return (
            <div>
                <BodyEnd>
                    <div ref={this.setRef}>open</div>
                </BodyEnd>
            </div>
        )
    }
}

class BodyEnd extends React.PureComponent {


    componentDidMount() {
        this._popup = document.createElement('div');
        document.body.appendChild(this._popup);
        this._render();
    }

    componentDidUpdate() {
        this._render();
    }

    componentWillUnmount() {
        ReactDOM.unmountComponentAtNode(this._popup);
        document.body.removeChild(this._popup);
    }

    _render() {
        ReactDOM.render(this.props.children, this._popup);
    }

    render() {
        return null;
    }
}

render(<App/>, document.getElementById('root'));

What is the expected behavior?

componentDidUpdate should fire after setRef.

In the example above, if you open the Console, you will see "hello" null. The null is supposed to be a reference to the <div> but it hasn't been set yet. Why not? componentDidUpdate is supposed to fire after the component has rendered and all refs are called.

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

React 16.2.0, Chrome 63 on Ubuntu 16.04.

Yes, this worked fine in React 15.6

More details:

This sounds a lot like this issue reported on BountySource for react-bootstrap/react-bootstrap (@gaearon commented on this), but it doesn't look like they filed an issue with React. I did manage to produce an SSCCE, so I'm reporting it now.

@mnpenner
Copy link
Contributor Author

It should perhaps be noted that this can be fixed with a portal:

export default class BodyEnd extends React.Component {
    
    constructor(props) {
        super(props);
        this.el = document.createElement('div');
    }
    
    componentDidMount() {
        document.body.appendChild(this.el);
    }
    
    render() {
        return ReactDOM.createPortal(
            this.props.children,
            this.el,
        );
    }
}

But the above was working in React 15.6, so it seems like a regression/backwards-compatibility bug.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2018

Major versions don’t have to be backwards-compatible (that’s why they’re majors 😉). I haven’t looked into your example in detail but my hunch is this is a known intentional limitation that comes up with nested ReactDOM.render() calls but solves many other issues. Our recommendation is to use portals for this.

@mnpenner
Copy link
Contributor Author

mnpenner commented Jan 17, 2018

@gaearon Right, but you said yourself,

if something wasn't mention in "breaking changes" in 16 blog post, then it being broken is probably a bug. Please report such bugs to us, don't just try to work around them or assume it's meant to be broken.

I assume you're referring to this post.

Perhaps I'm mistaken, and one of those bullet points covers this scenario, but otherwise I don't see it listed.

I encountered this problem when upgrading my project from 15.6 to 16. We didn't have any warnings so I assumed it was safe to do so.

Not that it's a big deal because I was able to work around it by converting that component to use a portal, but had it been a 3rd party component this would have been a much bigger problem.

Or maybe I was just misusing the lifecycle hooks in React 15.6 and it never provided those guarantees in the first place? Anyway, I'll let you guys decide.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2018

I agree this wasn’t called out specifically. I still don’t know if it’s a bug or not. (I didn’t have time to look into this issue yet and probably won’t have for a while.)

The point I was thinking about is:

ReactDOM.render and ReactDOM.unstable_renderSubtreeIntoContainer now return null if called from inside a lifecycle method. To work around this, you can use portals or refs.

It doesn’t match your description, but I think it might have the same root cause. The internal description of this change could be “React is no longer re-entrant, so it’s not possible to begin rendering a component tree while we’re already rendering a component tree.” But that’s hard to understand. So we provided one possible effect of this. There might be others—thanks for helping us discover them. In general the recommendation is to not use nested ReactDOM.render calls and instead use portals.

@alekseykarpenko
Copy link

@gaearon this bug is now 4 months old and still reproduces on 16.2.0, but you suggest to create another one at StackOverflow ;) Any chances to fix this?

@gaearon
Copy link
Collaborator

gaearon commented Apr 25, 2018

That StackOverflow post didn’t mention anything about a nested ReactDOM.render() call so it’s not obvious to me this is the same issue.

I will however edit my SO answer to better clarify this.

I’m closing this particular issue because it is expected with React 16 that if you do ReactDOM.render inside a lifecycle then those children will be attached after the current tree has rendered. The fix for this is to use portals instead of nested ReactDOM.render calls.

If you have another issue with refs not attaching in time and don’t have nested ReactDOM.render calls, please file a new bug with a reproducing case and I will take a look.

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

No branches or pull requests

3 participants