ethul / purescript-react-redux

MIT License
15 stars 6 forks source link

Fix some typing #6

Closed BartAdv closed 6 years ago

BartAdv commented 7 years ago

* Parameterize ReduxReactClass by an action type, change type parameters order to make it uniform with React-bindings Spec: props goes first, then state, and actions come last, as they're addition here

TODO: semigroupoid instance for Middleware - that seems to require Middleware to be newtype rather than type synonym, which has some ugly consequences...

Also, it's late already and I'm not 100% sure I've made everything right, but openened this PR so that you can play around/check. I've also modified Example app to include some (useless) different Action types showing how the composition works: https://github.com/BartAdv/purescript-react-redux-example/tree/wip

ethul commented 7 years ago

Thanks for putting together this PR! Looking good.

I will take a closer look and dig in this weekend.

Thanks again for working on this!

ethul commented 7 years ago

I've been digging into the changes and mainly I'd like to further discuss the changes to the Middleware type.

Is there a driving use-case that you have in mind for having the middleware call next with a different type of action from the type of the store's action?

I was thinking that perhaps the use-case could be if there was an external middleware published and a user wanted to incorporate it into their app. However, the user would still have to map between their action type and the external. Consider the following example.

data ExternalAction = External1 | External2

externalMiddleware :: forall eff action state. Redux.Middleware eff action state Unit ExternalAction ExternalAction
externalMiddleware _ next =
  case _ of
       External1 -> next External2
       External2 -> next External1

Then in your app you could have:

externalMiddlewareTo :: Redux.Middleware (Effect eff) Action State Unit Action ExternalAction
externalMiddlewareTo _ next = -- ...

externalMiddlewareFrom :: Redux.Middleware (Effect eff) Action State Unit ExternalAction Action
externalMiddlewareFrom _ next = -- ...

And compose them:

externalMiddlewareFrom `composeMiddleware` externalMiddleware `composeMiddleware` externalMiddlewareTo

I am not sure if the above is clear, but I suppose adding the extra action type parameter makes something like this possible. Whereas before, I don't believe something like this is would have been possible.

Do you think this would be the main use-case for adding the extra action type? Curious on your take.

BartAdv commented 7 years ago

I'll address other issues later on, but as for the driving use case for Middleware type I'll blatantly say - I don't have one. It was something that struck me when browsing redux docs - they use this to allow action to be function for example (redux-thunk) etc. So it's just to make it in line with what redux offers. But I'm pretty sure we don't need to rely that much on Middleware here, in PS, as we have Aff etc...

ethul commented 7 years ago

Thank you very much for making the updates. Looking good. Understood about the middleware. I am still thinking about this a bit.