Skip to content
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

withState hoc #33

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

kanitsharma
Copy link

  • Renamed withState combinator to mergeState
  • Added withState HOC
  • Updated combineEpics to work with withState HOC
  • Update tests with withState HOC

Here is an example of an epic with withState and mergeState

export default withState((s$, a$) => 
  compose(
    map(() => actionSpreader("START")),
    tap(console.log), // [ state, action ]
    mergeState(s$),
    select("FETCH_INIT")
  )(a$)
);

The inner function in withState is automatically uncurried so we can also it as

export default withState(s$ => 
  compose(
    map(() => actionSpreader("START")),
    tap(console.log), // [ state, action ]
    mergeState(s$),
    select("FETCH_INIT")
  )
);

Also, lets discuss if you want the function names to be changed

@kanitsharma kanitsharma changed the title With state hoc withState hoc Oct 21, 2018
@joshburgess
Copy link
Owner

Hmm, I'm not sure about the function names. I'll need to think about this a bit and get back to you.

Also, I was thinking that if this library went down this route of specifying what args should be included through higher order functions that it would also do it for other things, like getState, dispatch, possibly dependencies (like an API Client, etc.)

@kanitsharma
Copy link
Author

Hmm, I'm not sure about the function names. I'll need to think about this a bit and get back to you.

Yeah I probably thought so 😄

Also, I was thinking that if this library went down this route of specifying what args should be included through higher order functions that it would also do it for other things, like getState, dispatch, possibly dependencies (like an API Client, etc.)

hmm as for the getstate and dispatch, if the user is already using the not stricter API then he would get the getstate and dispatch object instead of state$.

For the dependencies we can use an approach similiar to redux-observables, passing the dependencies in the createEpicMiddleware and receiving it as third argument with the use of HOC

and since our HOC is not only providing the state$, so we need to name it accordingly, I will leave that to you 😄

@joshburgess
Copy link
Owner

joshburgess commented Oct 21, 2018

@kanitsharma Well, the idea was to change to just having a single API that everyone uses + using the higher order functions in an ad hoc sort of way to add functionality, where you only import and use them when needed. This would simplify the library in that everyone who uses it would work in basically the same way by default.

So, the two APIs would go away and instead we'd just have one API that by default takes in only an action stream. Then, for people who want access to the quick-and-dirty/escape-hatch-y getState/dispatch, they could pull in something like withStore or withMiddlewareApi or something that injects an object like { getState, dispatch } and then they'd have access to that.

Then, the same thing would be available for the state stream, like what you've shown here.

And there could possibly be other useful things as well. For example, we could allow the user to define dependencies upon middleware construction, like what you described above, but instead of ALWAYS injecting those dependencies we just provide access to them through a higher order function, like withDependencies.

I'm not sure whether or not this is the best route to go down, but that was the original idea. It has pros and cons.

Pros:

  • Simplifies basic usage of the library. Everyone starts with just receiving an action$ and that's it. Access to other things would require the use of HOFs.
  • It's quite flexible, because it's sort of an "a la carte" approach, where you only use what you want/need.

Cons:

  • It could be viewed as adding extra boilerplate. If you want access to a few of those things all at once, you'd need to use each HOF for each thing you need in every epic they are needed. This is sort of a trade-off of having that flexible/ad hoc sort of capability. Maybe, there could be something like a withAll (bad name) that was just a composition of all of the other HOFs that gave you access to everything by default and you just choose what to use on your own?

I'm not 100% sure what the best solution is off the top of my head, but I do think changing the library so that there is only one base API + optional flexibility is a good idea. Finding the right solution will probably require some experimenting with different ideas.

@joshburgess
Copy link
Owner

joshburgess commented Oct 21, 2018

Another, simpler, option would be to just skip the HOFs altogether and just always pass everything through:

epicArgs => ...

And then the user could use destructuring to pluck off the things they want:

({ dispatch, getState, dependencies, stateStream }) => ...

This is definitely the simplest option, but maybe not the best? Not sure.

I don't want to remove access to dispatch and getState altogether, but I would like to discourage use of them somehow, since they should be viewed as escape hatches rather than the default way to go about things, because you really don't need them.

@kanitsharma
Copy link
Author

I am all up for having different HOFs for different use cases, As I see it would be very exciting to compose the HOFs and make it all pointfree 😄

@joshburgess
Copy link
Owner

I'm also a fan of pointfree style, but there is a caveat, unfortunately. Pointfree style in TypeScript/Flow can be pretty annoying due to TS/Flow not being able to infer from right to left with compose. It requires a lot of extra type annotating that wouldn't be required in languages like Haskell, etc. :(

BUT, I do think it would be nice to support a pointfree style for those who are willing to annotate or people who are just writing in plain JS.

@joshburgess
Copy link
Owner

Side-note: I would like to rewrite in TypeScript due to getting more into it recently at work, etc. + the library already having TS type definitions, but this would probably also be a good time to add Flow types to the library. I think @most/core already has both TS & Flow types available.

@kanitsharma
Copy link
Author

I'm also a fan of pointfree style, but there is a caveat, unfortunately. Pointfree style in TypeScript/Flow can be pretty annoying due to TS/Flow not being able to infer from right to left with compose. It requires a lot of extra type annotating that wouldn't be required in languages like Haskell, etc. :(

Agree, but since @most/core doesn't provide the fluent style/chaining most of the users will have to use the composition + pointfree style so the HOFs pattern makes sense.

@kanitsharma
Copy link
Author

kanitsharma commented Oct 21, 2018

({ dispatch, getState, dependencies, stateStream }) => ...

and as far as this is concerned we can use the withall HOF that you talked about to achieve this kind of mechanism.

@kanitsharma
Copy link
Author

Update on this,
I was a little busy last week so didnt get a chance to work on it.
Gonna work on the HOFs this week 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants