reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

React ES6 Components #470

Closed BryanGrezeszak closed 8 years ago

BryanGrezeszak commented 8 years ago

Added React ES6 component support of Reflux.Component as discussed in the first half of this thread: https://github.com/reflux/refluxjs/issues/468

spoike commented 8 years ago

Looks okay though PR's with big features like these needs to have unit tests.

BryanGrezeszak commented 8 years ago

Added unit tests, and in the process made some improvements for server-side usage and updated the package info to be a new version of Reflux (0.5.0) and use the latest version of React.

cy6eria commented 8 years ago

Good job!

BryanGrezeszak commented 8 years ago

Added ES6 React idiomatic flavored Reflux.Store as well which integrates easily with Reflux.Component.

Also... \ crickets **

Feedback, anyone?

BryanGrezeszak commented 8 years ago

I realized that with Reflux.Component and Reflux.Store working together it would be pretty simple to make it internally track the state of all in-use stores in 1 place (Reflux.GlobalState) which could then be saved and set next time the app is started you could set Reflux.GlobalState to the previously stored state and it would automatically initialize everything to that state...so basically I added the ability to easily persist an application state across different sessions / and/or manually start the app in any state for testing purposes.

jamesmarrs commented 8 years ago

Hey @BryanGrezeszak thanks for all this! With automatically binding the state with the store and components (I see its optional), do you see any performance issues with the components constantly checking to see if the component should update if you mutate the state that may not even apply to the component? (Not talking about a todo app, but a more complex example that is mostly built out) Or would you just continue to break down your stores into smaller pieces? I know there are many answers, but looking to spark conversation about it. Currently my stores are setup closely to a 'per model' basis.

BryanGrezeszak commented 8 years ago

@jamesmarrs - It doesn't have to actually watch the state of the store for mutations. It just updates as the store does a trigger. And if you're using Reflux.Store to define your stores and then mutating with the store's setState within it then that is automatic. So no performance issues with checking for mutations. I'm not sure if that's one of the things you were meaning, but worth mentioning anyway.

As far as stores with data being updated that isn't relevant to the component it's attached to: the general idea is that would the the anti-pattern to how you'd set up stores/components.

You'd have a store per set of related data and then each component could be attached to just the stores relevant to it. However, should you decide to blur those lines a bit (we all know no pattern can ever be followed 100% of the time!) the component's shouldComponentUpdate still works like normal, since the Reflux.Component is using React's normal setState to mutate its state anyway. So the pattern, while extremely useful, is not fully strict.

The Reflux.GlobalState also comes in handy there because you can still get an object containing a map of your full overall state regardless of where all your stores are, and you can still use such a state object to set the default state of every store in the app automatically before mounting. So while this pattern does involve breaking up stores a little more...it also provides a great way to deal with more at the same time.

BryanGrezeszak commented 8 years ago

v0.5.test.zip

The link above is an example of what @jamesmarrs was asking about. In there is a small project with a dynamic bar graph.

There are 2 stores: one for the graph specs (the physical size of the graph) and the other holds the actual data the bars represent and the max data the graph is supposed to go to. By grouping alike pieces of the state into their own stores we make it so that we can attach them to appropriate components with no wasted component updates. Since a change to the graph size affects the container AND the bars themselves that store is attached to both. But since a change to the graph data only affects the bars themselves they are the only entities that also attach to the graph data store.

It's already compiled and runnable, so you can just open the HTML file in a modern browser and it will automatically run some actions on a timer and you can watch the effect. Then in the console you can read out the Reflux.GlobalState and see that it holds all the data from both stores automatically. Then if you pick through the source (which are in the PackagingJS dependency system...a little foreign to most but that's all it is: the module dependency portion of how I wrote this specific test, just don't pay attention to it, look at the classes inside the package statements) and you can see how the stores are set up and used.

esbenjuul commented 8 years ago

@bryangrezeszak really good job -thx.

jamesmarrs commented 8 years ago

@BryanGrezeszak Thanks for putting all this together!

BryanGrezeszak commented 8 years ago

@jamesmarrs - No problem. Reflux is a great tool, it just needs some love to play well with React again now that React changed stuff around. Glad to contribute.

Latest update implements the suggestion from @inli3u to improve the way error handling is done while testing for the presence of React.

BryanGrezeszak commented 8 years ago

If anyone has any other input I'd love to hear it.

BryanGrezeszak commented 8 years ago

Are there any concerns in particular that anyone has about merging this? If there are I can work on them. No point in letting it go to cobwebs; we may as well address them and improve on it.

philipgiuliani commented 8 years ago

When will you merge this? After this i'll finally use Reflux... đź‘Ť

corneldm commented 8 years ago

:+1: This is fantastic and keeps me in the Reflux camp. Thank you!

BryanGrezeszak commented 8 years ago

So...would it be accurate to assume at this point that merger is not going to happen?

lszomoru commented 8 years ago

what's the latest on this pull request? Having ES6 support would be a great addition!

devinivy commented 8 years ago

I talked with @spoike and I'm clear to help get this PR across the finish line. I plan to make serious headway on this PR this week!

How does everyone feel about the API? In particular I'm interested in this approach versus that of react-redux's connect() function when it comes to connecting state to presentational components. I see two ways to author a Reflux-aware React component as a class,

  1. Write a class inheriting from React.Component which provides Reflux-awareness. This is the current approach of this PR.
  2. Pass an instance of React.Component through a function that returns a new React.Component with newly-gained Reflux-awareness. This is the approach of react-redux's connect().

Barring decorators (which I am not personally interested in for Reflux) I can't think of any other way to approach this. Given that there are at least two options on the table, I'm hoping we can justify one option over the other or conclude that it doesn't really matter. One concern I have with the current approach is that a React component might want to inherit from multiple classes (imagine using Reflux with another library that similarly adds mixin-like functionality to a component, and imagine that library also taking a class-based approach).

@BryanGrezeszak thanks very much for your work– I look forward to merging this.

devinivy commented 8 years ago

There's a third option which is sort of a combination of 1 and 2. It would be a more traditional JS mixin approach, and might look like this,

class MyComponent extends Reflux.Component(React.Component)
{
    constructor(props) {
        super(props);
        this.state = {foo:'bar'};
        this.store = myStore;
    }

    render() {
        return <p>From Store: {this.state.storeProp}, Foo: {this.state.foo}</p>;
    }
}

The upside here is that if there's some other library that extends React.Component, it could still be compatible with Reflux,

class SpecialReactComponent extends React.Component {/* ... */}

class MyComponent extends Reflux.Component(SpecialReactComponent) {/* ... */}

This could include a purely presentational component that you created,

class StatefulComponent extends Reflux.Component(PresentationalComponent) {/* ... */}
BryanGrezeszak commented 8 years ago

There is most definitely no reason to complicate (and make less-idiomatic) the general usage of the API in order to account for special case functionality that we want to add.

If we want to build a function that can give us an extendable object that can extend off 3rd party classes instead directly off React.Component then we can do that...with its own method (Reflux.extendFrom(MyOtherClass) or something)...but there's zero reason to make that function BE Reflux.Component itself and therefore mess up the idiomatic-ness of the other 99.9% of usage by forcing the extra non-idiomatic boilerplate of Reflux.Component(React.Component).

I make the point of stating this in a comment (as opposed to just implementing it that way and submitting) because I think it's important that that mindset of 100% idiomatic syntax being the general usage API be cultivated. For normal usage there should be no extending while calling a function to return what you're actually extending, no decorators, no mixing functions, etc etc. Just extends Reflux.Component because that's minimum learning curve, minimum boilerplate, minimum points of possible confusion, and maximum idiomatic way of doing it.

As far as relating to the Redux API: People use Redux despite it's API. We (yes, we...I've done more Redux than I've done Reflux) use Redux because of the power it offers (much of that power simply resulting from the fact that it can integrate into ES6 style React)...but grumble about the ridiculous implementation the entire time. If you're having a thought of "well, Redux's API works this way...should mine?" the automatic answer is "probably not". Making this API have more in common with React itself than with Redux was absolutely not accidental on my part. :)

devinivy commented 8 years ago

There is most definitely no reason to complicate (and make less-idiomatic) the general usage of the API in order to account for special case functionality that we want to add.

Most of all I hope to see an easy way to endow an already-existing stateless component with state. I think this is a core requirement of fluxy development today. Can you give a simple example of that using Reflux.Component? From my point of view this would be one good reason to complicate the API slightly.

I'm not going to advocate like crazy for the third option, but there's an opportunity to provide sugar so that Reflux.Component(React.Component) can be Reflux.Component(). I don't mind your idea of offering both a class and a mixin for ES6 classes.

BryanGrezeszak commented 8 years ago

If I understand your first paragraph correctly that's literally what the API is optimized for, and it's specifically easy because of the idiomatic API mimicking the same concepts of how you already work with ES6 React anyway. If you've got an existing React component without state (fluxy or otherwise) and you want to add state to it from the fluxy environment in question (Reflux) all you'd have to do is change what you're extending (React.Component to Reflux.Component) and assign your store via this.store = MyStoreClass in the constructor (which is also perfectly idiomatic because that's the same concept of how you assign an initial state anyway, completely familiar and already understood). That's it. That component's state now reflects that store, updates state from that store just like it was a native piece of state to the component, etc. It's that easy.

As far as the second paragraph is concerned (with the Reflux.Component()): again, why? It takes away idiomaticness not only from previously learned React usage itself but also with how normal OOP class based JS works (it's not common to extend a function call like that, and React.Component() definitely isn't how it's done), and there's no reason for doing it that way because you can simply utilize a different function name instead. Why remove a simplicity in order to add a feature when you can literally just add the feature without affecting the previous simplicity at all?

That's what I'm trying to communicate here. I'm not trying to say don't add features...I'm saying why would you remove the idiomaticness in the process of adding features when there's absolutely nothing stopping you from adding the feature without removing the idiomaticness? It's like throwing away the $20 bill in your wallet before adding a $10 bill. Just add the $10 alongside...there's nothing mutually exclusive there...keep both! It IS possible to keep a simple API for simple usage while adding a more complex API for more complex usage. :smile:

corneldm commented 8 years ago

+1 to just extending Reflux.Component as implemented in this PR. I'm not at all pleased with Redux's API and would hate to see those patterns repeated here.

devinivy commented 8 years ago

If I understand your first paragraph correctly that's literally what the API is optimized for, and it's specifically easy because of the idiomatic API mimicking the same concepts of how you already work with ES6 React anyway.

I don't think it's what I'm thinking of, but correct me if I'm misunderstanding. I'm specifically thinking of the case that I hand you a particular stateless React component (say, a class) PresentationalComponent that simply takes certain properties and renders them, and you have to connect those properties to state without changing PresentationalComponent. This is essentially the only reason I'm considering the complication of higher-order components a la https://gist.github.com/sebmarkbage/ef0bf1f338a7182b6775.

It takes away idiomaticness not only from previously learned React usage itself but also with how normal OOP class based JS works.

My concern is that the feature of "making a React component stateful" doesn't technically fit into an inheritance model very well. It just irks me a bit when adding features to a class is shoehorned into inheritance when inheritance might not be the right fit. I think it's my nerd-brain trumping my ergonomics-brain, and I'm sorry about that. I think you are right– there clearly are times when this simplicity is preferable.

I'm saying why would you remove the idiomaticness in the process of adding features when there's absolutely nothing stopping you from adding the feature without removing the idiomaticness?

Fair enough! :)

TLDR, Now I'm down with the current API but am still concerned about the ability to add state to pre-existing, non-stateful components– I think part of the reason for that is that it doesn't fit into traditional inheritance. I will be exploring how to allow for that while maintaining the simple API promoted by this PR. Don't be surprised if it's in some ways similar to react-redux's connect(). Again, it wont be necessary to use that feature as suggested by @BryanGrezeszak. Thanks so much for the feedback and patience @BryanGrezeszak and @corneldm. My impression is that your views represent the greater Reflux community fairly well.

Expect a merge soon. 🎆

BryanGrezeszak commented 8 years ago

Here's the thing...

The reason you're noticing that my problem solving is about inheritance is because it's literally solving the problem that the Reflux API sucks in dealing with inheritance...which is a huge problem because the React API is moving to being about inheritance!

You're simply on a different problem. Reflux is plenty capable of having 2 different problems! Don't confuse them with each other.

The problem right now...the problem that, to be frank, is the reason why Reflux is all but dead, is that it absolutely sucks to integrate into your actual building of a React ES6 class. Since React is moving to ES6 classes exclusively, this is killing Reflux.

My API solves THAT problem and THAT problem alone.

Furthermore, my API is built on top of Reflux and does not alter the original API nor the core in any way. It is an addition only.

If you want to solve a problem dealing with integrating Reflux from OUTSIDE a React class without coding in that class at all...then do that. It will not require this API, nor would it have anything to do with this API. This API solves a different problem...the problem that happens to be what's killing Reflux right now.

If you want to build a connect function to make Reflux connect to an existing React class without modifying any code in that React class then do so. But that simply has nothing to do with an API that is for building React classes with Reflux, which IS what is missing from Reflux that is making Reflux die right now. If you try to somehow integrate your completely unrelated problem with my solution all you're going to do is get an API that is junk for both problems.

Reflux needs this API...and it needs your connect function too...however these are different problems that would be utilized from completely different parts in the code to solve completely different problems related to completely different usages...and frankly it would only be confusing to even try to have such different capabilities have much of anything to do with each other in the API.

You're looking at your popped up nail and trying to hit it in with my crescent wrench...meanwhile the other side of the building is falling off for want of a crescent wrench...

If you break my crescent wrench on your nail you're going to end up watching that other side of the building fall off while going "dang, wish I had a crescent wrench that wasn't all beat to junk" :smiley:

You go get your hammer, I'll refine my crescent wrench, then we'll have the best of both worlds :smile:

corneldm commented 8 years ago

Thanks, @devinivy, I appreciate your time assessing these changes.

I'm specifically thinking of the case that I hand you a particular stateless React component (say, a class) PresentationalComponent that simply takes certain properties and renders them, and you have to connect those properties to state without changing PresentationalComponent.

This reminds me of how the Redux community differentiates containers and components. For this case, if you wanted to connect state to a dumb PresentationalComponent, I would suggest creating a new container type (or whatever you prefer to call it — e.g. controller component) that extends Reflux.component. In practice, these are more specialized components that tend to go beyond just wrapping a single dumb component. They usually define callbacks, handle routing, etc.

In fact, once this PR is merged, I'll be more explicitly separating my components that extend Reflux.container from my dumb components.

devinivy commented 8 years ago

In my mind both of these features are requisites to bring Reflux into the present, and the relationship between them is that your class will probably look something like Reflux.Component = Reflux.ConnectClass(React.Component) inside the codebase. I always saw them as being intimately related as far as the code is concerned– sorry if I didn't communicate that well. Though I think they do solve related problems.

@BryanGrezeszak Just so it's clear, I think the work you did looks great, and I hope you didn't take my open discussion as being critical of the quality of your work or offend you in any way. It's so fundamental to these two features that I figured this was a good place to bring it up. But it seems that this might not be the place to continue the conversation. Rest assured, the next version will have your Reflux.Component class usable essentially as-is. Does that settle your concerns?

@corneldm I don't think those concepts are limited to the Redux community, and I value them greatly :) By the time of the release I expect you'll be able to go further and create stateless components that are simply connected to stateful parameters like actions and store properties.

Don't worry, 0.5 is going to be sweet as pie. Any more feedback outside the scope of this PR (the Reflux.Component class) can carry-on in https://github.com/reflux/discuss/issues.

BryanGrezeszak commented 8 years ago

Don't worry, it's not an offense issue...it's a passion issue.

I'm just a guy that comes from an OOP/class based programming background that is doing more JS pretty much because JS is moving that direction (especially with React) and I see the power in it growing because of that. I'm watching JS development start to blossom as a true powerhouse for even very large apps built by very large teams, especially with React.

I genuinely believe that the only thing holding JS app building back is that none of the flux architectures are moving that direction. In fact...they're only getting more and more "let's map this to that with custom functions, etc" than they were before. It's getting to the point where the flux architecture (redux, reflux, or whatever), which should be a breeze, is the only thing really slowing/disorganizing most of the projects I see. They have plenty of features...they just don't have super organized and idiomatic ways of using them. With a good solid idiomatic inheritance based solution to flux in React I really believe app building in JS will instantly make leaps forward.

I believe Reflux has the capability to implement such an API...and I have very real interest in that happening. I honestly think I can halve some of the development time/organizational issues I see by being able to properly class out things things in such an idiomatic way...and I think Redux is too far gone to go down that path and that Reflux is the last chance.

I'm glad to hear that my worries here have just been a communication issue and that simply extending Reflux.Component will allow this.store and this.stores to work idiomatically without further complication. I'm also hoping that the ability to idiomatically setState within stores and set this.store to a Reflux.Store extended class instead of just an instance, etc. etc. also holds in there.

Glad to hear it, and looking forward to what you put out. Honestly...I don't care if you throw out every bit of code I did and write it from scratch, as long as the API to it still works! I'm not really in it for the code writing credit...I just want a simple inheritance based API for flux to exist so that I can do some massive apps in an organized fashion!

devinivy commented 8 years ago

Consider it merged! There's some more work to be done before v0.5, but this is certainly the foundation of v0.5. Cheers, @BryanGrezeszak 🍻

corneldm commented 8 years ago

:beers: 🎉 Thanks for making this a reality, @BryanGrezeszak

BryanGrezeszak commented 8 years ago

Sweet.

Now lets get some big projects going with it, see if we can break it, and then improve it some more :smile:

corneldm commented 8 years ago

I'm been using Reflux.Component and Reflux.Store on production for several weeks without any problems (challonge.com signed in user dashboard and all tournament bracket pages). I do server-side rendering and preload/hydrate stores. Aside from needing to namespace all of my stores (needed for assigning multiple stores to one component) the migration was pretty straightforward.