-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: Have the actions argument be the last argument #4
Comments
I like the idea of the higher order function. I'll take a look at this soon. |
It's |
@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 |
@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. |
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:
and never access the store at all. I don't think I want to have to always write:
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 |
I definitely think the higher order function is the way to go with this.
Especially as it plays well with our other suggestion of having store be a
stream. You could have different hocs for different injections.
…On Wed, 24 May 2017 at 14:38 Josh Burgess ***@***.***> wrote:
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
<https://github.com/jshthornton>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACrzGtc0suIaaW8Sdh9nM6f7TkrX-np_ks5r9DLIgaJpZM4NUBFe>
.
|
@jshthornton That sounds good. So, we could have two to start off with? |
Yup. Should be easy enough to add support for these.
There are a couple of ways we can achieve this. Maybe the hoc signature is
to always take (store, actions). And in the middleware we check if the
function length is 1?
So something like this:
```
if(epic.length === 1) {
epic(actions);
} else {
epic(store, actions);
}
```
Then the code for withStore would look something like this:
```
withStore = (fn) => (store, actions) => fn(store,actions);
```
And then withState:
```
withState = (fn) => (store, actions) => {
// do some magic to convert store to a stream
return fn(state$, actions);
}
```
…On Wed, 24 May 2017 at 16:16 Josh Burgess ***@***.***> wrote:
@jshthornton <https://github.com/jshthornton> That sounds good. So, we
could have two to start off with? withStore and withState$?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACrzGqxmS1q9xi8GuuEO-8bGzkmfq3HAks5r9EndgaJpZM4NUBFe>
.
|
@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 This involved creating a custom 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 |
@joshburgess my concern with the |
@jshthornton It's the only way to get access to the complete store with the observable symbol on it, because Redux's Also, I only added to what Redux's About your concern, it's true that if another middleware library required a custom 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. |
@joshburgess It shouldn't be a custom import { createStore, applyMiddleware, compose } from 'redux'
import reducer from '../reducers/index'
...
const store = createStore(
reducer,
/* preloadedState, */
compose(
epicEnhancer,
applyMiddleware(...middleware),
/* any other enhancers */
)
) |
@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. |
@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
|
@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 */
)
) |
@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? |
@joshburgess Here you go: https://www.webpackbin.com/bins/-KlOKaRn8pKG8_6-l55Y I agree with @jshthornton that there is no need to mess with the |
@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? |
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. |
@jshthornton @vlad-zhukov Okay, I've got a working example of the In terms of actually using it, see here: Getting this right involved using Actually, I think we could have two. They would be similar to I also need to make sure this works properly when using 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 |
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 |
@jshthornton One thing I was thinking was that we could have 4 operators, like this:
where the first two take in only one argument, and the latter two take in 2 arguments (either 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 Maybe, we could have both those & and a separate
|
I like the concept, but the only downside is if I am developing an inner
observable that needs to grab the latest state.
Aka, listen for action, something happens, need to do some more stuff with
the latest state.
…On Mon, 5 Jun 2017 at 16:04 Josh Burgess ***@***.***> wrote:
@jshthornton <https://github.com/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 place.)
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)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACrzGvCe889CcQUdxAscyQ_pmM2XTCAvks5sBBjmgaJpZM4NUBFe>
.
|
@jshthornton You mean accessing the latest state multiple times (expecting different results) within the same Epic? |
Kind of. It doesn't even need to be multiple times. Often this situation
arises
```javascript
const myEpic = (actions$, store) => {
return r.pipe(
select(TYPE),
map((action) => {
return doSomeKindOfAsync()
}),
switchLatest(),
map((data) => {
const state = store.getState();
// Do some calculations here as things may have changed during that
time
// In theory this could be abstracted out by listening to other
actions that would have affected the state and this
// epic.
return action;
}),
);
};
```
…On Mon, 5 Jun 2017 at 16:43 Josh Burgess ***@***.***> wrote:
@jshthornton <https://github.com/jshthornton> You mean accessing the
latest state multiple times (expecting different results) within the same
Epic?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACrzGsatqownjpaJUebrEHjr5dSjCdLHks5sBCIQgaJpZM4NUBFe>
.
|
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 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 |
@jshthornton @vlad-zhukov I updated the https://github.com/joshburgess/redux-most/blob/enhancer/src/withLatestState.js |
@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 I also included an alias. So, this utility can be imported as either 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 Similarly, it's not great to have to pass in all arguments all the time with the 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 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 |
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. |
In monadic programming the iteratee is provided last. I am proposing that
actions$
is provided as the last argument and store is first. This could be achieved by checking the length of the provided function to the middleware, then providing the store if it is > 1.The other option would be to make it via high order functions, whereby you'd have a
withStore
. Which would essentially do it for you.Example syntax of both solutions:
The text was updated successfully, but these errors were encountered: