Fix some typing #6
Conversation
|
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! |
BartAdv
force-pushed
the
redux-class-action
branch
from
691bfd1 to
de9f690
Compare
BartAdv
force-pushed
the
redux-class-action
branch
from
de9f690 to
13e2391
Compare
BartAdv
force-pushed
the
redux-class-action
branch
from
13e2391 to
a5621f1
Compare
|
I've been digging into the changes and mainly I'd like to further discuss the changes to the Is there a driving use-case that you have in mind for having the middleware call 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 External1Then 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` externalMiddlewareToI am not sure if the above is clear, but I suppose adding the extra Do you think this would be the main use-case for adding the extra |
| @@ -106,19 +114,13 @@ exports.applyMiddleware = function applyMiddleware(middlewares){ | |||
|
|
|||
| var action = actionForeign.action; | |||
|
|
|||
| var result = middleware(middlewareAPI)(next)(action)(); | |||
| var result = middlewareB(middlewareAPI)(middlewareA(middlewareAPI)(next))(action)(); | |||
There was a problem hiding this comment.
I think something is not lining up here. I was trying to compose more than two middlewares, but it results in:
Cannot read property 'constructor' of undefined
| import Data.Either (Either, either) | ||
| import Data.Function.Uncurried (Fn2, Fn3, runFn2, runFn3) | ||
| import Data.Lens (Getter', Lens', Prism', matching, set, to, view) | ||
| import Data.Tuple (Tuple(..), fst) | ||
|
|
||
| import Prelude (Unit, const, id, pure, unit, (<<<), (>>=)) |
There was a problem hiding this comment.
Can we put Prelude as the first import? I think it's pretty common amongst PureScript libraries to have this up top. We could also have it as an open import (e.g., import Prelude)
| import Unsafe.Coerce (unsafeCoerce) | ||
|
|
||
| import React as React |
There was a problem hiding this comment.
I think I'd like the React import towards the bottom here like it was. Usually, I try to keep all the non-core imports below the core ones. I am considering core imports to be Prelude, Control, Data, etc.
| import Control.Monad.Eff (Eff) | ||
| import Control.Monad.Eff.Class (class MonadEff, liftEff) | ||
|
|
There was a problem hiding this comment.
Sorry to nit-pick, but can we keep the spacing between the imports?
| , applyMiddleware | ||
| , fromEnhancerForeign | ||
| ) where | ||
|
|
||
| import Prelude (Unit, (>>=), (<<<), const, pure, id, unit) | ||
|
|
|
|
||
| foreign import connect_ :: forall state props props'. Fn3 (state -> props -> Tuple state props) (Tuple state props -> props') (React.ReactClass props') (ReduxReactClass state props props') | ||
| foreign import connect_ :: forall state props props' action. Fn3 (state -> props -> Tuple state props) (Tuple state props -> props') (React.ReactClass props') (ReduxReactClass props props' state action) | ||
|
|
||
| foreign import dispatch_ :: forall eff action props state. Fn2 (React.ReactThis props state) action (Eff (ReduxEffect eff) action) |
There was a problem hiding this comment.
Should the order of the type parameters be eff props state action? This comment applies generally. There are something other functions where action is still before state. Not sure if we decided to reorder these too or not.
|
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... |
|
Thank you very much for making the updates. Looking good. Understood about the middleware. I am still thinking about this a bit. |
#* Parameterize
ReduxReactClassby 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 hereMiddleware, to reflect the fact that it can be composed and thus next middleware in chain can receive different action type.TODO:
semigroupoidinstance forMiddleware- that seems to requireMiddlewareto benewtyperather 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