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

No Upgrade Path #523

Closed andjd closed 7 years ago

andjd commented 7 years ago

Hi.

I'm working on a team that has a big project using Reflux going back to version 0.4. We're interested in progressively transitioning our codebase to ES6 class style syntax. Right now this transition is impracticable.

Trying to tie existing Stores made with Reflux.createStore to the new Reflux.Component classes is unreliable--the documentation specifically states that using the Reflux.Component methods only supports connecting to an ES6 Store. We've found that initializing data when connecting to old stores often fails and our component state is undefined. Additionally, under the new API for Reflux.Component, there are not methods that work like the Reflux.connect and Reflux.connectFilter Mixins. In order to have a smooth upgrade path, it's important to be able to put the content of our stores behind a chosen key, as was the pattern under the old API. We don't want to have to change the structure of the store, and require the updating of all subscribing components, whenever we create a new component using the new syntax.

The features we would need to be able to upgrade are:

BryanGrezeszak commented 7 years ago

You've definitely missed some parts of the docs.

For example, you state: "In order to have a smooth upgrade path, it's important to be able to put the content of our stores behind a chosen key"

There are multiple ways of doing this, depending on how much control you need. You can take full control of how state from stores maps via this.mapStoreToState. Or if you just need to filter what is brought in you can use this.storeKeys.

"the documentation specifically states that using the Reflux.Component methods only supports connecting to an ES6 Store."

Where? The new docs are all about the new ES6 syntax of course, but where does it specifically state that the old cannot be used? As far as I know the new docs don't even mention the old syntax existing.

Considering you're working with a mix of old and new, you could check out old docs from that transition period. They'll probably give some good insights.

Much of the example code from those docs is even written with a mix of Reflux.Component and old stores.

BryanGrezeszak commented 7 years ago

I'm closing this, but not for any of the reasons above. We can discuss all this stuff, but it should be done as individual issues. Trying to tackle multiple different concepts in single issues just turns into a cluster**** that doesn't do anyone any good. :)

andjd commented 7 years ago

here is the documentation that indicates that the new components require new stores: https://github.com/reflux/refluxjs/blob/master/src/defineReact.js#L159 Given that it sounds that the library is currently supposed to support mixing of legacy stores and new components, I will isolate the errors I've been seeing and file a specific bug report.

As for the comments about there not being an analog to Reflux.connect and Reflux.connectFilter, I am aware that using mapStoreToState presents a usable workaround (we tried it, that's how we ran into errors). The issue is that it's substantially more boilerplate code to write than what used to be a built-in one liner. With the change to the new syntax, the 'refluxy' way to write and connect to stores has changed, and the old 'refluxy' way doesn't work well with the new paradigm. Its unclear why this change in the API and patterns was necessary or beneficial.

While mapStateToStore can work, adding analogs to Reflux.Component that directly replicate the signature of Reflux.connect and Reflux.connectFilter would be very helpful because they enable us to continue to work with the established patterns we have in our application. I can create a new ticket for that change too.

BryanGrezeszak commented 7 years ago

I look forward to the bug reports you have for any problems you had with this.mapStoreToState, always good to find something that can be improved.

As far as wanting a 1 liner operation: this.store/this.stores does that, and this.storeKeys can filter whatever you want to only include part of the store.

As far as boilerplate difference mapStoreToState vs things like connectFilter, I honestly have no idea what you mean.

Old way:

mixins: [Reflux.connectFilter(store, "foo", function(change) {
    return change.foo;
})],

New way:

this.mapStoreToState(Store, (change)=>{
    return {foo: change.foo};
})

If anything there's less boilerplate, and more ability to do more with less since you can handle multiple properties at once, handle non-changes better, the ability to handle changes to props other than the one you want (i.e. ignore those changes to the store), etc, etc.

It's very simple. You're just defining a function where the input is what went into the store's setState, and the return is what you want to go into the component's setState.