Simplify #10
Simplify #10
Conversation
|
I've been experimenting with some breaking changes in this PR. I am considering merging this in soon, but I definitely open to any feedback you might have since you've all contributed to the repository. Thanks! PS - Updates to the example |
|
No objections here, looks good to me. |
src/React/Redux.purs
Outdated
| connect slens class_ = runFn3 connectFn Tuple (view slens) class_ | ||
|
|
||
| connect_ :: forall props state. Getter' state props -> React.ReactClass props -> ReduxReactClass' state props | ||
| connect_ :: forall props state. Getter' state props -> React.ReactClass props -> ReactClass' state props | ||
| connect_ slens class_ = connect slens' class_ | ||
| where | ||
| slens' :: Getter' (Tuple state Unit) props | ||
| slens' = to (view slens <<< fst) |
There was a problem hiding this comment.
why not slens <<< _1 _1 is in Data.Lens.Lens.Tuple
There was a problem hiding this comment.
I think you are looking at an outdated version here. connect does not use Getter' in the latest version.
src/React/Redux.purs
Outdated
| @@ -139,9 +135,9 @@ foreign import data REDUX :: Effect | |||
|
|
|||
| foreign import data Store :: Type -> Type -> Type | |||
|
|
|||
| foreign import data ReduxReactClass :: Type -> Type -> Type -> Type | |||
| foreign import data ReactClass :: Type -> Type -> Type -> Type | |||
There was a problem hiding this comment.
This will affect error messages. PureScript doesn't show qualified type names (or maybe it's just a problem of psa?). This might lead to some confusion when reading an error message.
There was a problem hiding this comment.
And for this, I think you are looking at an outdated version. This has been renamed to ConnectClass.
src/React/Redux.purs
Outdated
| @@ -39,7 +39,7 @@ import React as React | |||
|
|
|||
| import Unsafe.Coerce (unsafeCoerce) | |||
|
|
|||
| type ConnectClass' state props = ConnectClass state () props | |||
| type ConnectClass' state props = ConnectClass state (Record ()) props | |||
There was a problem hiding this comment.
why not just {} instead of Record ()?
There was a problem hiding this comment.
Good call. I will update this.
|
Thanks for all of your feedback! I want to make a few more small changes, but should get this out in a release soon. |
|
A little late to the party, but I like this too. Ditto on the move to newtypes. Thanks for continuing to experiment with this. |
|
Thanks for taking a look @pbrant! |
|
Just to note, release of these changes depends on purescript/purescript-record#7 |
|
@ethul quite a lot of changes! This means, unfortunately, I won't be able to dig into this stuff, as I'm not using this library at this moment. |
|
Will this be possible with this simplification? Or should I create a separate issue? Essentially I want to pass the |
|
@felixSchl Good call! No need to raise an issue. I will look into incorporating that into this PR. Thanks! |
|
@BartAdv Indeed! Revamping a lot in this PR. Thanks for the update! |
No description provided.