Proposal: Have the actions argument be the last argument about redux-most HOT 29 CLOSED

I like the idea of the higher order function. I'll take a look at this soon.

from redux-most.

It's (action$, store) => {...} right now, there is no need to break it. Higher order function should do the trick but it probably should be called differently than withStore.

from redux-most.

@vlad-zhukov The need to break it stems from the fact that most.js was designed to be monadic first. If it wasn't then everyone would just use rxjs, and therefore no need for this library as redux-observable exists.

In terms of naming of the higher order function you could either go with withStore, injectStore, or addStore to follow other library conventions.

from redux-most.

@vlad-zhukov @jshthornton I think it's okay to make a breaking change right now while still pre 1.0. I agree that this library should focus on consistently using a functional style, and I'm even okay with breaking away from redux-observable's patterns if we found a better way to use streams & redux together. I just want it to be a nice general purpose middleware library for integrating Most with redux projects.

I'm not sure how fast I'll get to this one though, so if anyone wants to show some examples of what the code might look like in Gitter or even open a PR for further discussion, feel free to go ahead.

from redux-most.

So, I thought about this more.... The reason the store comes as a second argument is because most of the time you don't actually need it. Most epics are just:

const someEpic = action$ = { ... }

and never access the store at all. I don't think I want to have to always write:

const someEpic = (store, action$) = { ... }

even when not using the store. Do you agree or no? I think this might make the higher order component idea the way to go. @jshthornton

from redux-most.

from redux-most.

@jshthornton That sounds good. So, we could have two to start off with? withStore and withState$?

from redux-most.

from redux-most.

@jshthornton @vlad-zhukov I have created a rough draft of what epics taking in a state$ (state stream) could look like. I haven't changed the argument order or created higher order functions yet. I simply created an alternative Epic API where epics can take in a state stream as the 2nd argument instead of the store.

This is meant to provide a fully declarative way to access the latest state from within epics. NOTE: dispatch is no longer available. However, dispatch isn't really needed when you use chain/merge/etc. anyway, since whatever actions emitted in the return stream of your epic are effectively "dispatched" to the store. This enforces a strictly declarative style, and I think it's pretty interesting.

This involved creating a custom applyMiddleware function, as Redux's applyMiddleware only provides access to getState and dispatch, not the entire store... This is explained in the comments. The code is in a new branch called state-stream. Please, see the following files:

https://github.com/joshburgess/redux-most/blob/state-stream/src/applyMiddleware.js

https://github.com/joshburgess/redux-most/blob/state-stream/src/createEpicMiddleware.js

https://github.com/joshburgess/redux-most/blob/state-stream/src/constants.js

https://github.com/joshburgess/redux-most/blob/state-stream/examples/navigation-react-redux/epics/stateStreamTest.js

from redux-most.

@joshburgess my concern with the applyMiddleware approach is that if any other tech requires their own implementation of applyMiddleware then we have effectively vendor locked the developer.

from redux-most.

@jshthornton It's the only way to get access to the complete store with the observable symbol on it, because Redux's applyMiddleware doesn't expose it. Otherwise, it's not possible to create the state stream, and this is only an optional feature for those who want it. I'm not removing the old style. I'm detecting whether the user is using redux-most's custom applyMiddleware and passing the epics a state$ if they are, but otherwise returning the old style "store" (really just an object with the store's getState & dispatch methods... a subset of the actual store).

Also, I only added to what Redux's applyMiddleware makes available. I didn't remove getState or dispatch. So, it shouldn't affect any other middleware.

About your concern, it's true that if another middleware library required a custom applyMiddleware to be used and the user wanted to use that middleware & this feature in redux-most there would be a problem to solve, but I don't think they'd be locked in. They'd just have to write their own applyMiddleware which incorporates the change I made + whatever change the other middleware requires. It's actually pretty easy. All you're doing is changing what's exposed. By default, only getState & dispatch are exposed. I could have exposed the entire store directly & then converted it into a stream in createEpicMiddleware, but I figured there was no need, so I just did that in applyMiddleware.

I think it's not very likely that this scenario would actually happen, and, if it did, I think it would be easy enough to explain how to solve the problem. Thoughts?

Again, this is only an optional feature, but I think it would be cool... as it really ties in with the more declarative/functional style of Most, and it's something redux-observable doesn't currently offer. We already have the ability to choose merging & chaining over directly dispatching. Why not also add a declarative way to get the most recent state? We don't have to force people to use that style, but just make it available for those who want it.

from redux-most.

@joshburgess It shouldn't be a custom applyMiddleware but a plain enhancer. All enhancers are composable, and applyMiddleware is just an enhancer too. Have a look at compose function from Redux.

import { createStore, applyMiddleware, compose } from 'redux'
import reducer from '../reducers/index'

...

const store = createStore(
  reducer, 
  /* preloadedState, */
  compose(
    epicEnhancer,
    applyMiddleware(...middleware),
    /* any other enhancers */
  )
)

from redux-most.

@vlad-zhukov Good call. I hadn't really looked at store enhancers much before other than using the Redux DevTools. I'll check it out. Thanks.

from redux-most.

@vlad-zhukov @jshthornton So, I've been playing around with trying to create a storeEnhancer, but it's not super clear to me how I would go about accessing the state$ in my createEpicMiddleware if I rely on Redux's applyMiddleware....... I need access to the state$ inside of createEpicMiddleware in order to pass it into the epic , and I don't see how I can do that.

applyMiddleware always comes after the enhancers, and it only exposes getState and dispatch... So, how would I make a new property accessible?

from redux-most.

@joshburgess Would something like that work?

import { createStore, applyMiddleware, compose } from 'redux'
import { createEpicMiddleware, createEpicStoreEnhancer } from 'redux-most'
import rootReducer from '../reducers/index'
import rootEpic from '../epics/index'

...

const epic = createEpicMiddleware(rootEpic)

const store = createStore(
  rootReducer, 
  /* preloadedState, */
  compose(
    createEpicStoreEnhancer(epic), // This one is the same as your custom applyMiddleware but 
                                   // only takes one argument - an epic
    applyMiddleware(...middleware),
    /* any other enhancers */
  )
)

from redux-most.

@vlad-zhukov Can you put something together to show me more? I'm not visualizing how I could solve the problem with what you showed... or at least not easily/in a way more simple than the custom applyMiddleware solution.

Here are the important lines for when the epics get called if you want to take a look:

https://github.com/joshburgess/redux-most/blob/master/src/createEpicMiddleware.js#L27-L32

I'd like to try to get this feature in soon, because I'm using redux-most @ work now, and I want to use this feature myself. lol. If we can come up with a way simpler than the custom applyMiddleware, I'm okay with it, but so far I think it's the simplest option and I don't think it's a bad way to go, considering it's completely optional (you only need it if you want the API where state$ is an available param in your epics)... Also, it's basically identical to redux's applyMiddleware outside of adding the state$ key. So, it wouldn't break anything for anyone. it's just a simple swap.

If Redux's applyMiddleware made the whole store available and not just getState/dispatch we wouldn't need to do any of this and could just covert it into a stream in the createEpicMiddleware layer...

@jshthornton What about you? Any ideas?

from redux-most.

@joshburgess Here you go: https://www.webpackbin.com/bins/-KlOKaRn8pKG8_6-l55Y
The example uses [email protected], so I guess epicMiddleware is not aware of the store passed into it. createEpicStoreEnhancer.js (need a better name for it lol) is a copy-paste of your custom applyMiddleware from the state-stream branch.

I agree with @jshthornton that there is no need to mess with the applyMiddleware from redux, or provide anything similar to it. With your own enhancer you can do whatever you want. And in the end createEpicMiddleware can be used with both applyMiddleware and createEpicStoreEnhancer.

from redux-most.

@vlad-zhukov I'll try this out when I get home tonight... if it works, do we offer both createEpicMiddleware (with the API we already have now) + createEpicStateStreamOrWhateverEnhancer for the alternate API with the state stream? I guess we could just always use the Enhancer only, but I'm not sure about ALWAYS using the enhancer as a replacement for createEpicMiddleware, because that seems a little weird... Most other middleware libraries don't do that, right?

from redux-most.

I still need to figure out what we're doing about changing the argument order/using higher order functions too... checking the function length (# of arguments passed in) and passing extra arguments before store$ when the length > 1 feels a little weird... like it might be a little too complex to explain to users? Maybe not. Not sure. Just not used to seeing that sort of behavior in other libraries.

from redux-most.

@jshthornton @vlad-zhukov Okay, I've got a working example of the state$ feature pushed up in a new branch called enhancer. In order to make it useful, I had to come up with a way to access the latest state from the state$ only when the select'd action comes through the action$.

In terms of actually using it, see here:

https://github.com/joshburgess/redux-most/blob/enhancer/examples/navigation-react-redux/epics/stateStreamTest.js

Getting this right involved using most's sample combinator, but flipping some arguments to be able to use it in the exact way I wanted to. This works fine, but it's a lot more work than simply calling getState. So, I think we should package up this functionality into another utility function (just like select or selectArray) to make it easier for the user, so that they can use state$ without having to think about things so much.

Actually, I think we could have two. They would be similar to ramda's zip and zipObj (I'm currently using the zipObj style in the example). They will basically just be curried functions that take in the state$ and the action$ and return a new stream that passes through either a zipped array, like [state, action] or a zipped object, like { state: state, action: action }, so that the next function in the pipeline will have access to both the state and the action in order to do whatever needs to be done.

I also need to make sure this works properly when using selectArray and passing multiple actionTypes, but that shouldn't be a big deal. I think the important thing is getting good names for these utility functions, so that the user doesn't have to think too much about it. The current sampleToZippedObj (or sampleToZippedArray, I guess) is an okay name. I mean, it describes what's happening fairly well, but it's long and conveys details the user doesn't really need to care about.

Can either of you think of good names for these?

Again, all of this seems like a lot of extra work & complexity just to avoid getState, but it's working pretty well so far, and I think it will be worth it if we name the utility functions well & get the API right.

from redux-most.

My only concern with creating an operator that exclusively works on state and actions is if somewhere in the epic we have lost the reference to actions the developer would not be able to use that function.

I'd recommend instead to have a function called withLatestState or sampleState which just takes another stream. Then just force the results to come back as an array [ value, state ]

from redux-most.

@jshthornton One thing I was thinking was that we could have 4 operators, like this:

select, selectArray, withStateSelect, withStateSelectArray

where the first two take in only one argument, and the latter two take in 2 arguments (either (action$, state$) or (state$, action$)... either way).

If we do this in combination with switching the order of the arguments and passing different things depending on the # of arguments passed in, like you mentioned before, we could actually skip needing higher order functions and just use the appropriate operator when we need access to the state. Really, we wouldn't even have to think about the order of the arguments if we did it this way, as the operator would take care of it. We could even make withStateSelect/withStateSelectArray work with with the normal getState API (without the enhancer).

The reason I like the idea of those operators is it would allow us to use a Pointfree style (with help from compose or pipe) where we don't have to mention the argument names in the function definition. (See my latest rewrites of the example epics to see this in some places.)

Maybe, we could have both those & and a separate withState or withLatestState like you're asking for and just leave it up to the user to pick which functions they want to use?

select, selectArray, withStateSelect, withStateSelectArray, withState (or withLatestState)

from redux-most.

from redux-most.

@jshthornton You mean accessing the latest state multiple times (expecting different results) within the same Epic?

from redux-most.

from redux-most.

Ahhh. Okay. I see what you mean. I haven't actually done something like that before. Usually my epics are in response to some sort of action, and if I need to access the state it's only one time either before or after any extra async occurs. hmmm...

@jshthornton I suppose that's a good point. If you don't use getState to grab the state right when you're about to use it, it's possible that the (slightly) earlier sampled state might be stale/out of date (if the state updated after your action came through)...

but it wouldn't make sense to always run the code every time the state changes either though, would it? It would happen all the time....

Hmmm... I think just starting with withLatestState as a standalone function is probably the way to go at first. I'll work on it soon.

from redux-most.

@jshthornton @vlad-zhukov I updated the enhancer branch with potential versions of the state$ utilities... I don't love the names, but I think this is better. What do you think?

https://github.com/joshburgess/redux-most/blob/enhancer/src/withLatestState.js

https://github.com/joshburgess/redux-most/blob/enhancer/examples/navigation-react-redux/epics/stateStreamTest.js

from redux-most.

@jshthornton @vlad-zhukov I've updated it once again. Now, it features only the function which packages the state & the other stream value into an array. I figured it made sense to delete the function that packages the state & other stream value into an object, because, this utility can be used with any stream, not just the action stream, and, with the array function, the value from the other stream can be named whatever the user wants to name it (via destructuring), whereas they would have to follow a naming convention with the object function.

I also included an alias. So, this utility can be imported as either withLatestState for those who want to be explicit or withState for those who just want a more concise name.

I think this is looking like the final API for this stateStream feature. The only thing left to figure out is the rearranging argument order/higher order functions issue. I've been thinking about this, and, although it IS best practice to have the iteratee passed in last, I'm not sure we're actually getting anything out of it here if we have to check the function length to know the # of arguments & the order to pass them in... because that means we wouldn't be able to use currying/partial application anyway, which is the main reason why it would make sense to pass the action$ in last.

Similarly, it's not great to have to pass in all arguments all the time with the action$ always coming last, because that prevents us from using a Pointfree style even when we don't need access to the store or the state$.

So, this really only leaves the higher order function option if we want to change the order of arguments. My question is, is it really worth needing the extra functions & the API change to include these higher order functions? Would regular users find it less convenient to have to use the higher order functions to access the store or state$? What do you guys think? Especially @jshthornton since you first suggested it.

See:

https://github.com/joshburgess/redux-most/blob/enhancer/src/createStateStreamEnhancer.js

https://github.com/joshburgess/redux-most/blob/enhancer/src/createEpicMiddleware.js

https://github.com/joshburgess/redux-most/blob/enhancer/src/withLatestState.js

https://github.com/joshburgess/redux-most/blob/enhancer/examples/navigation-react-redux/epics/stateStreamTest.js

from redux-most.

Closing this for now due to the dead discussion. I finally released the new API.

I'll open a new issue to link to this to maybe change the API again in the future to use a higher order function instead of what I'm doing currently.

from redux-most.