Issue with server rendering and HoC

[dozoisch]

Alright, I don't know if the issue is on the way I'm doing things, but I have a weird issue when using HoC on server side rendering.

My stores fetch their data automatically when asked for

ex:

const ConnectedApplyToEvent = connectToStore(ApplyToEvent, {
  event: store => ({
    events: store.getUpcomingEvents(),
  })
});

store.getUpcomingEvents() will return the events if they are already fetched, or will create the action to fetch them.

So when rendering server-side, I just do renderToString first. This create the actions needed, and once all the actions are finished, I do a second renderToString and the view then gets rendered with all data needed.

When the store dispatches the change event from loading, this function gets called:

https://github.com/acdlite/flummox/blob/master/src/addons/reactComponentMethods.js#L185-L190

function createStoreListener(component, store, storeStateGetter) {
  return (function () {
    var state = storeStateGetter(store, this.props);
    this.setState(state);
  }).bind(component);
}

The problem is that the setState is called on a non-mounted component which creates an error afterwards. That error then stops the dispatching and "everything breaks".

The only fix I could see was to check whether the component was mounted before calling the set state. And I'm using a custom this.mounted instead of this.isMounted() because isMounted is deprecated...

So

// reactComponentsMethod.js
componentDidMount() {
   this.mounted = true;
}

function createStoreListener(component, store, storeStateGetter) {
  return (function () {
    var state = storeStateGetter(store, this.props);
    if (this.mounted) {
      this.setState(state);
    }
  }).bind(component);
}

I don't know if there is another way of doing it?

[dozoisch]

This is the branch I'm using on my own right now.

dozoisch@77e7d48

[burabure]

im not sure if the following is the direct cause of your issue, but stores shouldn't fetch, do async or side-effecty things (that includes firing actions).

try putting your async/fetching in actions and passing those results to stores.

[dozoisch]

@burabure Alright, might be an issue with the way I'm doing it then.

I'm not sure about that philosophy'though. I've seen both implemented ways a lot in Flux-ies.

Anyway, if the stores don't fire actions, it means whether you need actions to know about store state (you don't want to be fetching data constantly for no reason) OR then every component needing the data needs to check if the data is already fetched, and then fire the actions if it's not.

If your stores don't do fire actions, you need to have another layer of functions (like the routerWillRun that checks if the data is fetched, and fires actions if it's not.) And you get to have a lot of code repitition that way. And it also means that you need the top handler to know about all of his child, instead of having components that needs data be connected to the store (with the HoC) and ask for his own data.

[burabure]

take a look at this presentation https://speakerdeck.com/jmorrell/jsconf-uy-flux-those-who-forget-the-past-dot-dot-dot-1

also here's an awesome checklist by Dan:

[dozoisch]

@burabure Alright, I can refactor the stores to be totally syncrhonous.

But where would you put the fireing of actions to fetch the data.

• You will need to have a routerWillRun function that needs to know about all of his childs, (or actually all the child that are not handlers). And this fires actions. You can then wait until theses actions have loaded the data into stores to do the server render. This means duplication of code that checks if the store has the data or not..
• You check if the data is available on willTransitionTo or fire the actions from there. Meaning you have duplication checks for every component who needs similar data (but are not necessarily on the same page). But that is pretty much the same as routerWillRun.
• You do the fetch on componentDidMount (like most examples) which means you are not server rendering, but client rendering.

I'm kind of lost on how to do it. I've been trying to figure it out in the past days, but fireing actions from the store was the simplest way I found to make it work both server-side and client side.

Any comp just ask for data, and it will eventually receive it. All you needed to change on the server was the actions (because they need to use a different api so that they fetch data from database directly and not from ajax requests), and you have to render twice. First time, the components auto-register the data they need, once it's loaded you render and send to client.

[bhj]

I ran into this with Reflux but according to the author it's OK for a store to fire an action, at least sometimes :p

[burabure]

@dozoisch I don't know what would be best for your app, but there's plenty of places you can fire the action on components; getInitialState or constructor, componentWillMount...

flux is very new, and the community is starting to figure where the footguns are; firing actions from stores is one because it can cause cascading updates very easily.

[dozoisch]

@burabure I guess that makes sense, I can change where actions fireing happens. There is still one issue though (that is the same).

The connectToStore register callbacks before the component is mounted. (It registers the stores callback on initialization.) And if I'm not mistaken, renderToString will initialize the components, but will never mount them. So if an actions completes, juste after a renderToString, the callback will fire setState on a non-mounted component. Which shouldn't be possible?

I'll do some more testing to see the exact lifecycle called when rendering with toString

[burabure]

from the react docs

Render a ReactElement to its initial HTML. This should only be used on the server. React will return an HTML string. You can use this method to generate HTML on the server and send the markup down on the initial request for faster page loads and to allow search engines to crawl your pages for SEO purposes.

If you call React.render() on a node that already has this server-rendered markup, React will preserve it and only attach event handlers, allowing you to have a very performant first-load experience.

so it seems to me that it shouldn't be an issue. but if you find any problems just post back

[dozoisch]

The error happens server side not client-side. It happens after the render to string, once the actions fnishes and updates the stores. The store tries to setState on a component that is not-mounted (but has been rendered with renderToString()). I'll try to investigate more to see why the callback is registered when the component is not "mounted" directly.

[burabure]

is difficult to be certain of where the issue is occurring without looking at the project itself. could you post the offending component code?

[burabure]

ok, I found the cause. renderToString is meant to be synchronous for now. here's the recomendation from pete hunt on this:

facebook/react#982

I think that (server side) data fetching is workable even in a sync world. You can just move the asynchronicity outside of React. You can even put the code for data fetching alongside your React component, just don't call render() until all the data is fetched and pass the data in as a top-level prop.

so I would move the data fetching to a helper-like js file on /utils and call that on actions and before rendering the component on the server.

[dozoisch]

Alright, I'll take a look at this. My goal was to be able to call renderToString first (which would fire the actions), I wait until actions have completed then call another renderToString with all the data now in the stores. Maybe I can disable the change event from the store when on server or something! I'll get back to you, thanks for helping.

[burabure]

I am glad I was able to help. Let me know how you solve it at the end =)

[dozoisch]

@burabure I've did some more investigations and connectToStore adds the
listeners before the componentDidMount. When static rendering
(renderToString for ex), the only lifecycle function called is componentWillMount.
DidMount and WillUnmount are never called. In connecToStore, the
listeners are added at inialization.

That means that if I fire actions at initialization, or at willMount it will do the exact same issue. That I have right now.

Solution I can see is to change the moment listeners are added.

At initialization, create a listeners map which is only "added" at
component did mount. This requires very few changes. You just push them in an
array and at didMount you go through and add the "change" event.

ex:

// instead of store.on("change", listener); you do:

this.listenersToAdd.push(() = > {
  store.on("change", listener);
});

then:
componentDidMount() {
  for (listenerToAdd of this.listenersToAdd) {
    listenerToAdd();
  }
  delete this.listenersToAdd; // or just this.listenersToAdd = [];
}

This way, stores won't get a setState event when not mounted. Because right now,
if for any reason, someone does a renderToString, then fires an action (from
any function) it'll just do a bad setState. (Because components are not mounted)

[dozoisch]

@burabure Any thoughts on that?