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

ReactDOM.render()/unstable_renderIntoContainer() doesn't return instance if called during an update #10309

Closed
seanlandsman opened this Issue Jul 27, 2017 · 27 comments

Comments

Projects
None yet
5 participants
@seanlandsman
Copy link

seanlandsman commented Jul 27, 2017

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

Bug

What is the current behavior?

ReactDOM.render and ReactDOM.unstable_renderSubtreeIntoContainer no longer return created React component instances

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/84v837e9/).

this.eParentElement = document.createElement('div');
const ReactComponent = React.createElement(this.reactComponent, params);
this.componentRef = ReactDOM.render(ReactComponent, this.eParentElement);

What is the expected behavior?

After the steps above this.componentRef should be an instance just created - it is now null with React 16 beta.

It's entirely possible that I should be doing something different now, but if so it's not clear what that should be

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

React 16 beta
Chrome
OSX

thanks

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

Seems like the same issue as #10294 (comment).

@bvaughn bvaughn referenced this issue Jul 27, 2017

Closed

React 16 RC #10294

@jlongster

This comment has been minimized.

Copy link
Contributor

jlongster commented Jul 27, 2017

This does seem related. I posted the small test case here that errors for me: https://github.com/jlongster/fiber-modal-error

In this case the react-modal library is calling renderSubtreeIntoContainer and also expecting an instance back.

I was going to file a new bug but this one seems like it?

@jlongster

This comment has been minimized.

Copy link
Contributor

jlongster commented Jul 27, 2017

Although it is weird because my above test cases works if you always render <Modal> instead of doing this.state.open && <Modal> like we do. So apparently is does get an instance back in the former case?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

@jlongster

So apparently is does get an instance back in the former case?

In your example, it only fails to get the instance on first render. This is why it only fails with {this.state.open && <Modal />} since that makes each render “first”.

@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

@gaearon Yes, it def looks related

I was working on a reproducible example too, but this isn't a trivial exercise trying to extricate only the relevant bits

Interestingly everything renders correctly as it did before, its just not returning an instance (which is important for actions down the line)

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

@seanlandsman

Can you please provide example? I just recreated your example in fiddle and it does return an object.
https://jsfiddle.net/dLqbryo6/

@jlongster

This comment has been minimized.

Copy link
Contributor

jlongster commented Jul 27, 2017

I will file a new bug with mine because it might not be related, feel free to dupe to this one.

@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

@gaearon Great, ok - I'll take a look now

I actually came to the same point that @jlongster did in #10294 - containerFiber.child is null in getPublicRootInstance

I'll take a look at your example and try see what's different to mine - thanks

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

I reproduced it!
https://jsfiddle.net/x7c7bdh0/

This only happens when ReactDOM.render is called during an update (e.g. in componentDidMount).

@gaearon gaearon added the Type: Bug label Jul 27, 2017

@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

Ah, great! I'll continue to see what I can find in the meantime though, but I am pleased you've managed to reproduce it!

@jlongster

This comment has been minimized.

Copy link
Contributor

jlongster commented Jul 27, 2017

I filed my bug here: #10310

Still not sure if this is related. I think renderSubtreeIntoContainer might never return an instance just like here, because the modal library only ever tries to access this.portal in componentWillUnmount here: https://github.com/reactjs/react-modal/blob/master/src/components/Modal.js#L129

If we render it like <Modal isOpen={this.state.open} /> componentWillUnmount will never be called and we don't realize that this.portal is null.

@gaearon gaearon changed the title React 16 beta Render - Components instances no longer returned ReactDOM.render()/unstable_renderIntoContainer() doesn't return instance if called during an update Jul 27, 2017

@aweary

This comment has been minimized.

Copy link
Member

aweary commented Jul 27, 2017

I believe the issue is related to how Fiber tracks scheduled work. Specifically: https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberScheduler.js#L1408-L1420

When the root ReactDOM.render call is made, no work is scheduled so performWork is called, which eventually sets the child property on the Fiber. But when ReactDOM.render is called in componentDidMount, there's already work in progress. So that !isPerformingWork check evaluates to false and the work isn't scheduled. That means performWork isn't called and child is never set.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

This sounds correct to me. I’m not sure how to fix this.
I think @acdlite should have the most context on this.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

This is also slightly related to #8830, as we’d bump into this problem in async mode anyway.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 27, 2017

We currently force top level renders into a synchronous mode for compatibility with this case normally. As a legacy mode.

The core issue is that we don't want to support reentrant renders. That's why render is not synchronous in life-cycle methods. It'll return what has already been rendered, which initially won't be anything yet.

The idea is that these use cases should ideally switch to using Portals instead. Perhaps we should warn about these use cases and recommend switching to Portals?

@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

@sebmarkbage if portals are the way forward for this sort of thing, where can I find documentation/information on how these might be used?

Alternatively, is there a way to get the instantiated component later? In our particular use case we don't necessarily need it immediately - some point later might be acceptable

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

if portals are the way forward for this sort of thing, where can I find documentation/information on how these might be used?

render() {
  return ReactDOM.createPortal(MyComponent, domNode);
}

You can mix that with regular elements too, e.g.

<div>
  <button />
   {ReactDOM.createPortal(<MyComponent />, domNode)}
</div>
@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jul 27, 2017

Alternatively, is there a way to get the instantiated component later? In our particular use case we don't necessarily need it immediately - some point later might be acceptable

There technically is although it's a bit weird.

ReactDOM.render(<MyComponent />, node, function() {
  console.log(this); // instance
});

Note that this won’t work with arrow functions.

Or more familiar:

ReactDOM.render(<MyComponent ref={instance => {
  if (instance) {
    // do something
  }
}} />, node);
@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

Ok, I tried the portal approach and this does return something, but it isn't the instance I expect here.

Perhaps I can describe my usecase - we develop a grid/table library whereby users can supply React components which our library then creates dynamically at runtime and inserts to render within the grid.

Optionally users can then access these components at runtime and invoke methods on them (via an api we expose).

For example, a user might supply a ToggleColorComponent with a method called "toggle" - they can get the instance of ToggleColorComponent for a given cell and invoke the toggle method (typically via an external input such as a button etc).

Does this make sense?

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 27, 2017

@seanlandsman If you don't need the component to be visible yet, I'd recommend doing requestIdleCallback(() => ReactDOM.render(...)) instead so you can defer the work until later.

If you do need to be visible at the same time, then I'd recommend using the Portal.

The tricky part of the Portal API is if you want to render into something that you're also rendering. In that case, I'd recommend manually creating the container DOM node and appending the instance in componentDidMount.

class MyComponent extends Component {
  myContainer = document.createElement('div');
  attachPortal = (parent) = {
    if (parent) {
      parent.appendChild(this.myContainer);
    }
  }
  render() {
    return [
      <div ref={this.attachPortal} />,
      ReactDOM.unstable_createPortal(<MyOtherComponent />, this.myContainer)
    ];
  }
}
@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

The second option described above (using hte callback) works for me - I had actually tried this but as I used an arrow function this hadn't worked. Trying a normal function as you described @gaearon did the trick.

@sebmarkbage We do actually need the component to be visible when we render it, it's just that we don't necessarily need access to the instantiated component immediately, just at some point after (maybe even really really soon afterwards, but not necessarily immediately)

If I use the callback approach described above (which works fine for me), is this likely to be a supported mechanism going forward? It suits us fine, but what we're trying to do here is ensure forward compatibility for when fiber is released

thanks all for your help!

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 27, 2017

@seanlandsman Just note that that technique doesn't work as well with async rendering as Portals. So it's not as future proof.

@seanlandsman

This comment has been minimized.

Copy link
Author

seanlandsman commented Jul 27, 2017

Ok, I understand, thanks - close but no cigar

@jlongster

This comment has been minimized.

Copy link
Contributor

jlongster commented Jul 28, 2017

I'm assuming you can do ReactDOM.unstable_createPortal(<MyComponent ref={portal => this.portal = portal} />, domNode) ?

For the renderSubtreeIntoContainer method at least, since that was marked unstable, if you don't think this should be fixed that's probably fine. render seems a little more risky though.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jul 28, 2017

The original plan was to special case this and pre-render just one level deep. E.g. just instantiate the class. However, that turned out to not be that useful distinction. Because what you really want is the whole tree to be rendered so that you can safely call findDOMNode, get refs and such. The instance you get also wouldn't have had its componentDidMount called so it wouldn't be fully initialized.

So I don't see a way to fully fix this without making it reentrant. However, that's a big architectural change that goes in the opposite direction of where we want to go (which is fully async).

silentcloud referenced this issue in react-component/m-dialog Sep 28, 2017

medelbou added a commit to signal/sprinkles-ui that referenced this issue Jan 23, 2018

fix(Popover): Fix Popover react 16 JS error.
Fix Popover react 16 JS error by providing a call back to ReactDom.render. see
facebook/react#10309 (comment)

45

ryanballa added a commit to signal/sprinkles-ui that referenced this issue Jan 24, 2018

fix(Popover): Fix Popover react 16 JS error. (#46)
Fix Popover react 16 JS error by providing a call back to ReactDom.render. see
facebook/react#10309 (comment)

45

ryankshaw added a commit to instructure/tinymce-a11y-checker that referenced this issue Sep 19, 2018

Handle react 16’s async rendering
this is a workaround for react 16 since ReactDOM.render is not 
guaranteed to return the instance synchronously (especially if called
within another component's lifecycle method eg: componentDidMount). see:
facebook/react#10309 (comment)

Test plan:
* when rendered into canvas-lms using react16, it shouldn’t throw errors

ryankshaw added a commit to instructure/tinymce-a11y-checker that referenced this issue Sep 19, 2018

Handle react 16’s async rendering
this is a workaround for react 16 since ReactDOM.render is not 
guaranteed to return the instance synchronously (especially if called
within another component's lifecycle method eg: componentDidMount). see:
facebook/react#10309 (comment)

Test plan:
* when rendered into canvas-lms using react16, it shouldn’t throw errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment