Skip to content
This repository has been archived by the owner on Jun 3, 2019. It is now read-only.

NOTICE: componentWillMount and this starter kit #409

Closed
ctrlplusb opened this issue Apr 6, 2017 · 8 comments
Closed

NOTICE: componentWillMount and this starter kit #409

ctrlplusb opened this issue Apr 6, 2017 · 8 comments

Comments

@ctrlplusb
Copy link
Owner

ctrlplusb commented Apr 6, 2017

We use react-async-bootstrapper to bootstrap your application on the client/server sides so that things like async components (and data in the features/react-jobs branch) can be pre-resolved.

Your React application will go through the following process:

  • Passed into asyncBootstrapper - this tries to mimic the behaviour of a server side render (to a degree) by walking your react component tree. It has to call componentWillMount along the way as this is expected behaviour in renderToString/renderToStaticMarkup calls. Generally people put things like state initialisation etc in the componentWillMount, so this will be needed so that the render functions of your components can be called with the required state it needs to return it's result (i.e. children). The process continues down the "tree" until the bootstrapping process is complete. This may sound expensive but all we are doing is creating and throwing away object instances - no DOM creation/interaction/manipulation occurs.
  • After the bootstrapping process has complete your application is passed to the render/renderToString functions for "normal" rendering (with all the global state given to it by the bootstrapping process).

I have found the above working awesomely for me in large production projects that use Redux as a state management tool. However, I have become aware of other libraries (e.g. MobX) that bind some "event listeners" within the componentWillMount call. This will cause unexpected behaviour given our bootstrapping process. This is because the bootstrapping process never calls componentWillUnmount, again mimicking the behaviour of a typical renderToString/renderToStaticMarkup call. Unfortunately libraries that generally do event binding in componentWillMount do the unbinding within componentWillUnmount. As we never call this the event listeners for the bootstrapping process live on, and then things like MobX state updates will cause things like forceUpdate errors as the associated components do not exist.

I am considering extending the asyncBootstrapper function to allow you to pass a prop stating that componentWillUnmount should run. This may improve interoperability with these types of libraries, however, it could cause issues on others. We will have to experiment together.

@birkir
Copy link
Collaborator

birkir commented Apr 6, 2017

For anyone concerned, @ctrlplusb has already made a patch that fixes the problem.

A boggling question from my perspective is... what is the side effect of calling componentWillUnmount by default for all components?

@ctrlplusb
Copy link
Owner Author

@birkir one thing I can think of is those components that rely/expect some sort of dom/event binding to exist. hopefully they check this is the case before doing any sort of disposal.

I do wrap the call with a try catch to avoid fallovers though. It will be interesting to see how this fairs for us.

@xiao-hu
Copy link

xiao-hu commented Apr 6, 2017

hmm.. without setting the componentWillUnmount flag in asyncBootstrapper, the following also works:

useStaticRendering(true);
asyncBootstrapper(app).then(() => {
    useStaticRendering(false);
    render(app, content);
});

@ctrlplusb
Copy link
Owner Author

Hey @xiao-hu
I have never heard of useStaticRendering - got any references on this?

@ctrlplusb
Copy link
Owner Author

Aha, just found that. Yeah, that sounds like a much better solution for mobx. Officially supported and you can use it around the full server side render too. 👍

https://github.com/mobxjs/mobx-react#server-side-rendering-with-usestaticrendering

@xiao-hu
Copy link

xiao-hu commented Apr 7, 2017

That's right. Thanks for the very detailed description of the root cause!

@diondirza
Copy link
Contributor

Guys, after applying this to my production app using useStaticRendering like @xiao-hu mentioned earlier is the best solution in my case

useStaticRendering(true);
asyncBootstrapper(app).then(() => {
    useStaticRendering(false);
    render(app, content);
});

Using options { componentWillUnmount: true } on asyncBootstrapper function still produce forceUpdate warning on console if you have component that changing state inside componentWillUnmount event.

@ctrlplusb
Copy link
Owner Author

Thanks for getting back to use about this @diondirza

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

No branches or pull requests

4 participants