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

API Additions #481

Closed BryanGrezeszak closed 7 years ago

BryanGrezeszak commented 8 years ago

Added a few API elements to better make use of what's going on with Reflux.GlobalState

Main two additions:

Reflux.getGlobalState and Reflux.setGlobalState methods. Former allows a deep clone of the Reflux.GlobalState which won't continue to mutate (so you can easily store snapshots of the global state) and the latter allows you to set the global state at any point (including with partial global state objects that will only override the stores/properties represented). This basically allows an API for state based time travel.

Also added is the Reflux.initializeGlobalStore method. With expanded global state ability comes more usefulness in working with ES6 stores both in and outside of components. This allows instantiation of a proper singleton instance of a store and setting it up for tracking by the global state without any need to attach to a component.

Tests, documentation, and a lot of code commenting was also added.

devinivy commented 8 years ago

Checking it out. @spoike any initial thoughts?

BryanGrezeszak commented 8 years ago

Anyone have any input on this?

Pretty minimal API additions, but they make for some pretty huge additions to the capability of the library. Being able to get/set full or partial global states at any time opens up large possibilities.

spoike commented 8 years ago

LGTM. Haven't had time to check it out yet but judging by the diffs it looks OK. ons 24 aug. 2016 kl. 13:18 skrev Bryan Grezeszak notifications@github.com:

Anyone have any input on this?

Pretty minimal API additions, but they make for some pretty huge additions to the capability of the library. Being able to get/set full or partial global states at any time opens up large possibilities.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/reflux/refluxjs/pull/481#issuecomment-242059965, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAkJUTBjsButN_RXnOv9QG5VqZYSlypks5qjESVgaJpZM4Jk_Pd .

BryanGrezeszak commented 8 years ago

Anyone else have any thoughts? I've used this version in a small project thus far without issue. It's about as stable as any first release of API additions can be as far as I have been able to determine. I think we're good to go.

BryanGrezeszak commented 8 years ago

I also noticed in my recent usage that the ability to attach a store and only update off of certain parts of its state would be convenient. So I added an ability to do so on a component-by-component basis via setting this.storeKeys in the component constructor.

itkach commented 8 years ago

only update off of certain parts of its state

isn't that achieved via connectFilter?

BryanGrezeszak commented 8 years ago

@itkach - No. This is the ES6 API which does not use mixins.

itkach commented 8 years ago

ok, but then shouldn't it be this.storeFilter to match this.store and this.storeFilters to match this.stores? this.storeKeys looks useful, but limited

BryanGrezeszak commented 8 years ago

@itkach - It does not work the same as the older storeFilter method, so there does not seem to be any reason to name it after it. That would only lead to confusion.


Later Edit: I'm talking about connectFilter here and getting things backwards thinking that he's saying something completely different. Basically disregard, I hadn't slept enough at this point.

BryanGrezeszak commented 8 years ago

@itkach - FYI: I am open to another name if we want to look into that.

I just don't think it should be storeFilter since we shouldn't have multiple functionalities that work very very differently but have the same name. The implementation of storeFilter involves passing functions to a function and such, and is very imperative, whereas storeKeys is literally just an array of the key names you want to keep and is very declarative. Getting the two confused would definitely lead to whatever you're trying to do not working at all!

My logic behind the naming was that you're declaring the keys (in the key/value pairs) from the stores' states that you want merged into the component state, hence storeKeys. But if we can think up a better name I'm all for it.

itkach commented 8 years ago

@BryanGrezeszak I understand what this.storeKeys does and I'm not suggesting a different name for it. The name is fine, but functionality is limited. To me it seems that this effectively sets up a specific filter function, and so is a limited form of what could previously be achieved via connectFilter in mixins-based api. Why not set a filter function, say via this.storeFilter = filterFunc, or, if multiple stores are connected using this.stores={storeName1: store1, storeName2: store2}, with this.storeFilters = {storeName1: filterFunc1, storeName2: filterFunc2}.

itkach commented 8 years ago

The implementation of storeFilter involves passing functions to a function and such, and is very imperative

Did I miss something and there's already a storeFilter in the new ES6 api? I'm not seeing it

BryanGrezeszak commented 8 years ago

@itkach - Those are perfectly good suggestions for API features. They just aren't this API feature :)

The current ES6 API for merging store states into components is a more declarative API. Are there people out there that want fully imperative stuff to manipulate the connection between store and component far beyond a simpler declarative API? Sure...including yourself. And eventually I'll get around to giving them that too. But I only have so many hours and right now I want to make sure the more declarative ways of doing things are solid and robust.

The ES6 API simply merging the state of the store into the component it's attached to I find this gets you 95% of what you need with good enough architecture planning, but the ability to optionally limit that merging to just a couple keys within a store's state fills in that last 5%, so I'm adding it. That way your declarative API does all you need while still being pants-on-head simple to learn and use.

That's all this feature is, rounding out some bits with this declarative API.

Fully imperative stuff will come eventually.

BryanGrezeszak commented 8 years ago

Did I miss something and there's already a storeFilter in the new ES6 api? I'm not seeing it

I was just referring to the original connectFilter and incorrectly calling by the wrong name because I was getting all backwards with what was going on in the conversation. My bad.

itkach commented 8 years ago

I was just referring to the original storeFilter.

What's original storeFilter? Did you mean Reflux.connectFilter from mixin api?

more declarative API.

I'm not sure what's imperative about declaring that there is a filter to be applied.

Filter function allows to re-shape data received by component, or convert values, or extract parts of state deeper than one key. In comparison, the proposed this.storeKeys api is not very compelling.

BryanGrezeszak commented 8 years ago

@itkach - see my edit to that, I was getting all backwards in what I was calling what between our comments.

BryanGrezeszak commented 8 years ago

@itkach - As for the second part: it is very much imperative for an API to have you supply a function that determines via its own logic how to filter things and gives the result, absolutely. That's not a bad thing...it's just not the side of things I'm working on right now.

Think array reverse and sort. I can reverse an array by just supplying the appropriate function to do so to sort. But reversing is commonly enough needed that having it's own declarative way is plenty worthwhile. In comparison reverse isn't very compelling...yet it's probably more commonly used by the average JS person...because it's an easy declarative method for a common issue.

Similar here. The general nature of how the ES6 API tends to push the architecture leaves one hole that I've come across as far as declarative usage: simply being able to say "only take these properties from the store". I've mostly found it with tinier components that can piggyback off of stores used by larger ones but that don't need all their data, and I've seen it a lot. It's a tiny feature with a big return, worth its own declarative concept.

It also in no way prevents a more imperative method like that you suggest from being implemented, nor is it intended to. I'm simply not on to the more imperative stuff yet.

BryanGrezeszak commented 8 years ago

FYI:

There are people getting confused about versions out-in-the-wild. I'm getting confused questions about it. People are getting Reflux from other sources (npm, cdn's, etc.) and saying "this is the same version number as the repo, but none of these features are here!"

At this point there are, that I know of, at least 3 versions of the same (numbered)version of reflux in existence, with completely different API features and bugfixes present.

When I did my original ES6 additions I assumed we'd use some of the basic concept of semantic versioning and upped the middle digit to 0.5.0 since it was non-breaking API additions. It was decided that we don't want to call it 0.5.0 yet. I disagree with breaking from semantic versioning, but it's not my repo. But even if we don't want to use semantic versioning I highly urge there to be some form of version changing whenever we update the main repo here. Two different "versions" of the same version should not exist.

Since you guys want to do version changing your own way I haven't done anything with it in this PR, btw. That's why I'm letting you know here. Should you decide to merge this, I recommend you update version numbers in some manner. Do it however you want to do it, but please have different numbers!

BryanGrezeszak commented 7 years ago

As promised, a mapStoreToState method is now in with full control over mapping store states into component states. And of course includes documentation and unit tests.

itkach commented 7 years ago

Nice, thank you. Since this works the same as Reflux.connectFilter from mixin-based api, perhaps it makes sense to call it that? Or better yet, have a single this.connect to take store, state key and optional filter function. That would combine functionality of Reflux.connect and Reflux.connectFilter. In mixin api, Reflux.connect also could grow optional filter filter function and Reflux.connectFilter be deprecated, so we'd have the same clean, compact api both in ES6 and mixin-based use cases. What do you think?

BryanGrezeszak commented 7 years ago

@itkach - I guess to me it doesn't seem like it operates that similarly to connectFilter, just has similar capabilities.

It could be expanded to cover the original API as well, but it would have to work slightly different.

The reason for that is that the way this works is that when you use .setState({foo:'bar'}) in your ES6 store, it passes that object to your callback mapping function so that you can map just actual changed properties and only re-render as needed. This works because the ES6 stores have state in a common way (on a .state property) and call trigger in a common way when you use the store's setState (passing whatever you set via setState).

In the original API you really can store state however you feel like and trigger however you feel like. So because of that there would still be slight differences in its operation and it would include requiring certain patterns be followed for what you pass to trigger and possibly to how you store state in your store. But it's definitely something that can be discussed.

FlorianWendelborn commented 7 years ago

Is there any chance we can merge and publish this? I desperately need this mapping function and ES6 support via npm. It really sucks that I have to include this repo in package.json.

In addition to that, this project starts to look quite dead, except for the work done by @BryanGrezeszak.

That being said - if this doesn't get merged to npm - can anybody recommend a ES6-compatible alternative to Reflux? One that wouldn't be that different so that rewriting would be quite fast?

BryanGrezeszak commented 7 years ago

@dodekeract - For my own usage I've got all my newest work on Reflux published to npm and bower as reflux-edge. Feel free to use it. Then if this stuff gets merged all you'll have to do to be official is remove -edge.

I've been using it for some pretty huge-scale React projects and it's been doing great. It holds everything that's here in this PR plus a Reflux.Component.extend feature that I'm experimenting with.

FlorianWendelborn commented 7 years ago

@BryanGrezeszak thank you so much. That's exactly what I need.

eek commented 7 years ago

@BryanGrezeszak It's looking great! Great addition! 😀

BryanGrezeszak commented 7 years ago

I don't know if anyone is ever going to ever come back and merge anything, but just in case I figured I'd bring it up to date with my reflux-edge repo just in case.

spoike commented 7 years ago

Sorry about me being away from the project for so long. I've been pinged to add @BryanGrezeszak as member to reflux projects and publish rights to npm. Is @BryanGrezeszak and everyone else OK with that?

eek commented 7 years ago

@spoike, since I pushed you guys with emails into making @BryanGrezeszak a collaborator, I'm more than happy with it, and will be even happier if he wants to take this project to move it forward! :D

FlorianWendelborn commented 7 years ago

@spoike I'd be so happy to see him as the new maintainer or at least a member with merge & publish rights. Since this project is basically inactive except for him, I actually really regretted choosing it.

He seems to know his stuff and understand reflux completely. Please give him the rights he needs (assuming he's willing to maintain this).

BryanGrezeszak commented 7 years ago

I can't guarantee I'll be around all the time, but having some privileges to help keep things up would definitely be welcomed.