Documentation Help

[BryanGrezeszak]

I've updated the docs (and there's been a major version increase).

I'd like to ask for help in going over the docs and adding to them and improving them. Especially to @eek and @dodekeract who have been so involved and helpful recently. Feel free to test out the new major version (some big changes...I don't doubt hiccups), and do PRs for documentation improvements.

Thanks!

[BryanGrezeszak]

I know I'm not that good guys...there's gotta be some majorly important concept I'm missing in the docs. Feel free to suggest and/or PR.

[FlorianWendelborn]

@BryanGrezeszak sorry, I can't really comment on this anymore since I switched to redux due to the memory leak issue I had.

Only thing I immediately noticed is that you decided to use var instead of const. I suppose that's intentional, but what's the reasoning?

In addition to that I couldn't find anything on server-rendering. Since I had memory leaks with that I suggest to include enough documentation to allow users to implement it without leaking memory.

[BryanGrezeszak]

I thought you were going to open an issue about that? I don't even have info on what's happening in your memory leaks...so I can't really work on them :)

I don't like const at the moment. It used to be function level scope in some browsers, but ES6 defined it as block level scope (like let). So new browsers are gonna use it as block scope. And I just plain don't want block level scope most the time. And since babel is turning it into var anyway, it's not like we're getting any speed boosts. I just don't see any advantage in it yet. One day...but I'm not hopping on it yet.

As far as server rendering...I guess I honestly don't know what is different about server rendering with Reflux? I would assume if you know how to server render React, then using Reflux doesn't change that...or am I wrong on that? If you've got some good info/tips about server rendering with Reflux then feel free to pass 'em to me, I'd love to have them.

[FlorianWendelborn]

@BryanGrezeszak As far as I understood with server rendering the issue is that Reflux uses componentWillMount instead of componentDidMount for attaching the subscribe listener.

That means that there are references between the components and stores, which (as far as I understand) creates the issues I had.

I decided not to open an issue since it's easier for me to just switch, writing a reproduce wouldn't be paid by my client and take a lot of time, and reflux appears to be broken by design. I'm quite certain that I didn't do anything odd and every time someone tries to use Reflux for server-rendering it will result in the same issue I had.

Also, if I did do something wrong then an example is needed, as I consider myself to be experienced with React/Flux/Node and I obviously didn't manage to implement it properly.

If I didn't do anything wrong then an example is the perfect repro for the bug.

[BryanGrezeszak]

@dodekeract - If there's no references between components and stores then how would components have data from the stores? That would be true for pretty much any flux implementation. So I highly doubt it's an unsolvable problem. I just haven't even tried to solve it yet, because I thought you were going to submit an issue with some code that replicated the problem. If not then I'll just try to replicate it myself.

[FlorianWendelborn]

@BryanGrezeszak as far as I know that's what getInitialState is for.

[BryanGrezeszak]

@dodekeract - I'm a little confused by what you mean there.

I'm assuming you mean the constructor, because we don't have getInitialState anymore in React, it's been deprecated and replaced by constructors in ES6.

But either way, It would still need reference to the store somewhere/somehow in order to get data from the store for initial state. It's no easier/harder to clean up references from getInitialState/constructor than it is from componentWillMount.

[FlorianWendelborn]

@BryanGrezeszak oh, didn't know they removed that. I'm using ES6/7 only so I didn't notice it's gone. :)

[FlorianWendelborn]

@BryanGrezeszak here's Facebook saying that you shouldn't do subscriptions in componentWillMount.

[BryanGrezeszak]

@dodekeract - I understand that subscribing during server rendering is likely where the leaks are starting, since componentWillUnmount never happens to remove them. And solutions to make sure all subscriptions get removed server-side is probably the first thing I'd look to do. And honestly, I don't think it'll be that hard.

But I'm trying to understand how you're saying getInitialState/constructor solves that? It still runs during server rendering.

If this allows for some sort of solution, then I don't want to miss it by simply misunderstanding you...but as far as I can tell it's just changing around where we're adding the leak, not actually doing anything to help it.

[FlorianWendelborn]

@BryanGrezeszak there is no need to subscribe at all on the server side. That's why it's usually done in componentDidMount. getInitialState/constructor is only there to get the initial state. There is no need for listening to any other actions/changes besides the initial hydration.

[BryanGrezeszak]

@dodekeract - User's constructor state setting would then kill off what's set from the store.

    constructor(props)
    {
        super(props); // <-- state from store would get set during super
        this.state = {}; // <-- brand new object is set which doesn't have store state
    }

You can't set state before the user defined constructor. And in the chain, the user's defined constructor goes last.

[BryanGrezeszak]

Just pushing the state to the component during componentWillMount and not actually listening until componentDidMount is an option though. So the concept could fly. But I don't think the constructor itself can be utilized for this.

[FlorianWendelborn]

Just pushing the state to the component

That's exactly what getInitialState/constructor is supposed to be used for. 🙂

In server rendering (as far as I understood it) you're actually supposed to

1. hydrate the store
2. render it once

Therefore you actually don't need to listen for changes from the stores and adding a handler for getInitialState/constructor is exactly the place where the components can grab the initial state.

---

User's constructor state setting

Is that compatible with reflux? I was under the impression that reflux is supposed to be the only thing that manages the state for every component it's assigned to.

[BryanGrezeszak]

Is that compatible with reflux? I was under the impression that reflux is supposed to be the only thing that manages the state for every component it's assigned to.

Definitely compatible. It's been designed/documented from the start that you can still have your own component specific parts of the state, and use them like you would in any normal component.

Reflux tries not to break normal React usage. If you have a normal React.Component and just flip the extends to Reflux.Component then the grand majority of the time it will work the exact some out of the box. It tries as much as possible to only augment it with new capabilities (like this.store, etc.) without removing any of the old.

---

That's exactly what getInitialState/constructor is supposed to be used for.

Exactly. And that's why we need to leave that capability intact.

I think you're looking at it wrong. For us (as people making tools to be used with react, as opposed to actually building projects with react components) the best practices of what you're supposed to do when making react components are abilities to make sure we maintain (because the people using our library are making react components, so they need those best practices to remain unchanged).

So if we remove the ability of the person making react components to set this.state in the constructor for the sake of doing some of our own state stuff in the constructor...then we're not following react's guidelines, we're breaking them. 🙂

---

Therefore you actually don't need to listen for changes from the stores and adding a handler for getInitialState/constructor is exactly the place where the components can grab the initial state.

This is true. It just doesn't require getInitialState/constructor to remain true.

One can push state in WillMount and then start listen in DidMount and it would serve the same functionality. I don't know if the DidMount stuff in general is what I'll do, but it's an option on the table. I don't like the idea of needing to super the DidMount part of the lifecycle in general, because it's very commonly used...so I'm not committing to it by any means. But we'll see where it goes.

[BryanGrezeszak]

This has been open long enough as far as the request for doc help.

And the convo about memory leaks that it turned into has been solved at https://github.com/reflux/refluxjs/issues/507

So closing this.