Skip to content

Start API change for connect decorator #15

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

Closed
wants to merge 2 commits into from
Closed

Conversation

Keats
Copy link

@Keats Keats commented Jul 30, 2015

Start of some work on #1

Rename select to slicer to be clearer (and goes well with the existing variable this.state.slice which is the result of calling slicer

Add binding for action creator, a tad different from what was suggested in the thread:

@connect(
     state => ({string: state.foo}),
     dispatch =>({
        append: (...args) => dispatch(appender(...args))
     })
 )
@connect(
   state => ({string: state.foo}),
   {append: appender} // equivalent to MyActionCreators in a real app (import *)
)

I haven't done merge yet as there are still things unclear imo (what happens if you get several part of the state etc) and I wasn't sure whether I missed something from the discussion so I'll add merge if everything is ok and clear

@Keats
Copy link
Author

Keats commented Jul 30, 2015

We should also probably not export the components and only have the provide/connect decorators exposed

@@ -4,7 +4,7 @@ import shallowEqualScalar from '../utils/shallowEqualScalar';
export default function createConnectDecorator(React, Connector) {
const { Component } = React;

return function connect(select) {
return function connect(slicer, actionCreators) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change it to selector. We call them selectors in the Redux documentation, as well as in reselect API.

We can rename slice to selectedState.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Either way, slice isn't a public API so we may as well call it anything.)

@gaearon
Copy link
Contributor

gaearon commented Jul 30, 2015

Hey, thanks! I intended to start in a slightly different direction.

Can you please completely remove <Connector>?
You would need to move most logic from it inside connect.

After this chunk of work is ready, we can bikeshed on action creator binding a little bit more. :-)

@aaronjensen
Copy link

@Keats Not sure if this will help, but here is the implementation I did as a shim from fluxxor to redux to aid in us converting: https://gist.github.com/aaronjensen/95e505cdcbbe9d46019d

This skips the need for Connector and has merge (you can ignore the subscriptions parameter, that's fluxxor specific).

@gaearon one thing we noticed is that there are times when we want to pass the props to the selector function. Can you think of any reasons that'd be a bad idea?

Also, it looks like you envisioned not proxying the props along in the default merge, so your default merge would just be (state, actions) => ({ ...state, ...actions }). Is that right? Or should it include props? (in my experience, if you include props, state should probably be merged after props or confusing things happen with nested containers and liberal {...this.props} passing.

@gaearon
Copy link
Contributor

gaearon commented Jul 31, 2015

one thing we noticed is that there are times when we want to pass the props to the selector function. Can you think of any reasons that'd be a bad idea?

You can make your merge powered by reselect as well.. I think. Am I missing something? Then your third parameter can enjoy the same kind of memoization benefits.

Also, it looks like you envisioned not proxying the props along in the default merge, so your default merge would just be (state, actions) => ({ ...state, ...actions }). Is that right?

I'd rather have (state, actions, props) => ({ ...props, ...actions, ...state }). (Thanks for the tip regarding props btw.)

@gaearon
Copy link
Contributor

gaearon commented Jul 31, 2015

Thank you for the effort @Keats! I'm closing in favor of #16 because it's more feature complete and already avoids using Connector.

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.

3 participants