facebook/react

After server-side renderToString() component claims it isMounted()

Closed this issue · 14 comments

If you keep a _this reference at the componentWillMount() lifecycle method of a component that is then rendered with renderToString(), then _this.isMounted() === true later on. The documentation however claims that server-side components aren't really mounted; since componentDidMount() is never called.

I think this behavior changed in React 0.14 but that might be wrong.

jimfb commented

I'm a little curious how you ran into this, but yeah, looks like a bug to me.

I noticed this while working on react-transmit. Its universal components started to spit warnings when doing server-side rendering: those components should call setState() only on the client-side, but started doing that on the server too since upgrading to React 0.14. Traced that behavior and found that isMounted() now returns true on the server.

Note that componentDidMount() is still not called, so my current work-around is to keep track of the mounted state manually.

(Very unlikely to be a good first bug.)

I start fixing it but I don't understand something. If we allow to call componentWillMount here then why _this.isMounted() should return false? The cleanest way to mark component as unmounted is simply unmount it after renderToString but it will call componentWillUnmount.

We have below options:

  • Custom mount, which will not mark component as mounted (but it should not call componentWillMount then?)
  • Simply unmount component after render (which will call componentWillUnmount) - the best way IMO
  • Another hack, which will not break previous tests

@Laskos componentWillMount is called on the server, where componentDidMount is not. Therefore the component is never really mounted on the server. That is the big difference between those two distinct lifecycle methods. @spicyj Referring to the suggested fixes: To change the behavior of invoked lifecycle method calls would mean breaking existing components. For example by calling componentDidMount and/or componentWillUnmount some components will attempt to run client-side code that they specifically put in those skipped-on-the-server lifecycle methods. IMHO (I don't have in-depth knowledge of React core) it's not worth it to fix a small regression like this by overhauling the component lifecycle, affecting examples, tests, documentation et cetera.

jimfb commented

@RickWong is correct, we certainly don't want to call componentDidMount or ComponentWillUnmount on the server. The lifecycle is that the component will mount, but the component is not yet mounted (isMounted() should be false) on the server side.

Just dropping by to say I am observing this is still the case under React 0.14.7

It looks like the current implementation just checks for the existence of __renderedComponent which is actually assigned in performInitialMount before mountComponent is called. If isMounted should only be true after componentDidMount then it seems like it'd be better attach a flag in invokeComponentDidMountWithTimer indicating it mounted?

ping @spicyj what do you think about doing something like that? ^

it'd be better attach a flag in invokeComponentDidMountWithTimer indicating it mounted?

(Unrelated, but invokeComponentDidMountWithTimer is not a good place to do anything—it is only called in DEV. It specifically exists so we don’t pay for unnecessary indirection and/or code size.)

Thanks @gaearon I didn't notice that. Looking at reconciler/ReactCompositeComponent.js#L337-L343

    if (inst.componentDidMount) {
      if (__DEV__) {
        transaction.getReactMountReady().enqueue(invokeComponentDidMountWithTimer, this);
      } else {
        transaction.getReactMountReady().enqueue(inst.componentDidMount, inst);
      }
    }

componentDidMount calls are enqueued in the transaction in all cases. I would expect isMounted to be set right after componentDidMount is called, so perhaps it could enqueue a function that calls componentDidMount and also sets a flag used by isMounted

// inline arrow function and flag name just used to illustrate the gist of it
transaction.getReactMountReady().enqueue(() => {
 inst.componentDidMount;
inst._isMouted = true
}, inst);

Then isMounted can check that flag instead of __renderedComponent.

isMounted can have many meanings and there's no clear correct one so I'm inclined to leave the current behavior even though it's a little bit confusing. Non-createClass classes do not support isMounted anyway.