React warning while implementing ES6

[jamesmarrs]

Hello,

I have two ES6 'Reflux.Component's that both get state updates from an ES6 'Reflux.Store' by setting this.store = myStore in the component. I receive this warning when navigating between the two with react-router:

warning.js:36 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op.

no warning is given by removing the this.store = myStore from one of the components and navigating between the two components with react-router.

Does the ES6 update want me to break up my stores and make them more granular per component basis? Or am i just missing something in the unmount process to achieve this functionality with no react warnings? (I am starting to migrate my current refluxjs application to ES6 syntax) Thanks in advance!

[BryanGrezeszak]

Can you get some code together that duplicates the issue?

I do have a few projects that operate with components with stores mounting/unmounting regularly without seeing this, so it's not as simple as that to duplicate. There must be some other element to it.

Once we have that I'll look into it. Maybe it's a bug, maybe it's just something we need to document better about usage...I dunno. We'll figure it out.

[jamesmarrs]

Thanks for the response @BryanGrezeszak, I am going to do a bit more research on my end first, was curious if anyone has seen anything similar while implementing the ES6 Reflux. Then Ill post some code or a solution ~

[BryanGrezeszak]

Fair enough. I can tell you that it definitely doesn't exist in all cases (as I have projects where it doesn't). I also can tell you that theoretically there's nothing wrong with having larger stores. It helps to use either mapStoreToState or storeKeys to just take sections (just from an "avoiding unneeded re-rendering" standpoint), but there should not be any reason you couldn't even just do a redux style 1 store to rule them all thing even if you wanted. That's not my personal recommendation of flavor, but it should be possible. So if issues come up from doing that then it is something worth at least looking into addressing.

Let me know what you come up with and I'll be glad to contribute what I can.

[jamesmarrs]

@BryanGrezeszak I took some time ripping out the bones of my codebase with matching versions, etc, into an example project to replicate my issue and everything seems to work fine (no warnings). I am not sure why the warnings are popping up with the ES6 implementation, but it must something else in my codebase. I think something isn't getting cleaned up correctly on my end. Appreciate the fast response.

[FlorianWendelborn]

This happens when you overwrite componentDidMount and componentWillUnmount.

[jamesmarrs]

Thanks for the comment @dodekeract, this did make me catch a difference from the example I was modeling after. @BryanGrezeszak I noticed that examples in the docs don't contain super() in the constructor of your Reflux.Store classes. I am unable to get babel to compile it without super() in the constructor. Is there a particular complier/preset you are using alongside webpack? (have es6linter turned off)

[BryanGrezeszak]

@dodekeract - Good call. I didn't think of that conflict. (note after the fact: it's componentWillMount that's used internally, not componentDidMount)

@jamesmarrs -

Fix for now is just do it manually:

// in your componentWillUnmount, you can essentially "super" like this:
Reflux.Component.prototype.componentWillUnmount.call(this);

That should do it in theory.

What do you guys think, obviously it should at very least be documented to be able to do stuff like this, but should we be looking for an API feature to make this easier? On one hand easier is better, on the other hand that might be getting a little too magic when there's a 1-liner for the concept anyway.

on constructor super: - There needs to be a super call if you're doing anything with a constructor, because without it there is no this to attach to. So that's a mistake in the docs.

I don't often use a super in my stuff because I use ES7 and assign the props to the class without the need for a constructor usually, so it's like

class MyStore extends Reflux.Store
{
    state = {foo:"bar"}
}

So it's easy for me to forget the super because it's out of habit for me. I'll make sure to find it in the docs and change it.

[jamesmarrs]

@dodekeract @BryanGrezeszak had some time today look into it, that was definitely the issue, really appreciate the help from both of you. After looking at the source code and checking out the warning call stack, it definitely makes a bit more sense. The one liner made the warning go away. I dont see any difference in why its not needed in componentDidMount method though? Everything still seems to work fine even though its overridden, did I miss something in the source?

[BryanGrezeszak]

Actually internally it hooks up the stores on componentWillMount, not componentDidMount.

That's the better one to use because A) any state changes don't require re-renders yet, so we can do all our stuff for initial state without consequence, and B) it's the only part of the mounting lifecycle that happens for server-side rendering, making ES6 stuff work server side.

[FlorianWendelborn]

@BryanGrezeszak is it possible that using componentWillMount instead of componentDidMount is what caused the server-rendering memory-leaks I experienced? (before completely removing reflux as a dependency due to them)

[BryanGrezeszak]

componentDidMount wouldn't actually do anything server side. Anything that tried to use it to hook stores to components server-side would just come out blank. So using componentDidMount would solve any memory leaks...but just because it would make nothing happen at all...so I don't think that's what you're looking for :)

Whatever's happening there may or may not be happening within that componentWillMount part of the lifecycle, but it's not from the choice of using that hook itself. It's actually the only option, as well as the best option since it can do its stuff as initial state instead of your component starting out with blank state and then re-rendering as we mixed in the store state (which is what would happen if done on componentDidMount.

More likely it's just the fact that most server-side implementations are gonna be building their scenes from scratch on every client request, and there's some data being left behind from each time that happens. So likely we're gonna end up with some manual solution where you call a method after ever server-side render and it tries to go through and kill all the data in Reflux at the moment.

[FlorianWendelborn]

@BryanGrezeszak Yes, but IMHO the issue appears to be that reflux mounts on the server-side, but never unmounts since componentWillUnmount is never called. If there is no way to fix this behavior then you should add a disclaimer that reflux is not safe for server-rendering, as it will heavily memory-leak.

[BryanGrezeszak]

Sorry, I edited some extra on there (probably while you were typing):

More likely it's just the fact that most server-side implementations are gonna be building their scenes from scratch on every client request, and there's some data being left behind from each time that happens. So likely we're gonna end up with some manual solution where you call a method after ever server-side render and it tries to go through and kill all the data in Reflux at the moment.

[BryanGrezeszak]

So yeah, pretty much. Saying Reflux mounts is a bit of a misnomer (I know...one I've used many times) because really all Reflux is is data like the state in the component, so it gets made before the mount part of the lifecycle just like state in React gets made before the mount part of the lifecycle, and that's how server-side works without actually mounting anything.

The difference is that React is the part you're calling when you actually build your markup server-side...so it knows to build the data, make the string and return it to you, then dump the data.

Reflux on the other hand just has to build it...then wait...because it has no idea when you're gonna actually call the React stuff to build using it, or whether you already did...or will 10 seconds from now...or whatever.

So we'll need to end up adding a method to the API pretty much just for the special case for you to tell it "Yah, did my rendering, done with it now, you can dump your crap", so that it knows to clean itself out.

[FlorianWendelborn]

@BryanGrezeszak Would it be possible to let reflux automatically drop any references when it detects that it's running on node? (Possibly using a setting/option/flag for that, so it won't break anyone's code)

EDIT: Actually, wait a minute - why does reflux even need to bind on the server-side? Isn't it supposed to use getInitialState for getting the initial state? Why would it have to force componentDidMount instead of componentWillMount?

[BryanGrezeszak]

It would cause race issues. Because you're building the data for your rendering of the markup, and it has no way to know whether you're done rendering that markup or not. Even if we could detect the render we couldn't detect the intent (what if they want to render, change 1 part of the state, then render again?).

So it pretty much has to be manual, because the dev is the only person that knows when they're done rendering out markup with the data they've currently got built.

[jamesmarrs]

@BryanGrezeszak componentDidMount was a typo, late night sorry. Meant to say where does componentWillMount auto bind in the source. Sorry!

[BryanGrezeszak]

@jamesmarrs - Both of the two component lifecycle methods for the ES6 stuff are in the defineReact.js file in the reflux source.

@dodekeract - Can you open a different issue for your memory leak stuff? I closed the other one we discussed this stuff on because that one was someone else's issue which really was about the Keep.js file in reflux-core, and that got fixed and therefore deserved to be closed out. But yours is deserving of its own issue to works towards.

[FlorianWendelborn]

@BryanGrezeszak sure, I'll try to find time for that. (I'd like to include a repro, so I'll need some time)