New API for connectDecorator implementation #16

gnoff · 2015-07-31T06:38:22Z

Alright,

Here's my take on the new connectDecorator API. Normal Args examples should all work from @gaearon 's comment in #1

API

decorate = connect(
  (State => StateAny) | null,
  ?((Dispatch => DispatchAny) | actionsObject),
  ?(StateAny, DispatchAny, ParentProps => Props)
)

MyConnectedComponent = decorate(PlainComponent)

//annotation syntax
@connect(
   (State => StateAny) | null,
  ?((Dispatch => DispatchAny) | actionsObject),
  ?(StateAny, DispatchAny, ParentProps => Props))
Component

//component with decoration
Component

//original component
Component.DecoratedComponent

~~I didn't delete createConnector but I did move createConnectDecorator to createConnectDecoratorDeprecated until we can decide if we should just delete these.~~

the old Connector and connect decorator have been deleted

I have tested through mocha only so no guarantees it is 100%

I'm happy to make changes/work on it etc... as opinions change on best api going forward if it's not ready to merge yet.

State is handled like Connector did on any notification from the store. shouldComponentUpdate will bail if the slice is the same

actions are bound using the provided dispatchBinder (2nd arg) only on component mount. Not sure if this is what you were aiming for in the API but it certainly reduces the number of binds to a lot less than every render and it seemed find since the typical use case for dispatchBinder is to simply bind dispatch to the action creator methods

merge is called on every render, reading stateProps and dispatchBinderProps from component state (not store state) and taking whatever current component props as the 3rd argument.

~~finally the decorated component is rendered with this.props, and mergedProps in that order.~~
If you want choose to provide a custom merge function then only the props returned from merge will be added to the underlying component

I also wrote a util for wrapActionCreators that fits the dispatchBinder argument signature and delegates to redux/bindActionCreators. This is called form connectDecorator if the actionCreators argument is a plain object

…Connector

…rge and puts sequential results of such on props of decorated component. Connector is intended to be deprecated. older connectDecorator is now called connecctDeprecated

gaearon · 2015-07-31T07:03:02Z

This is looking good so far! Can you please remove the old “deprecated” code for cleanness? I don't want to ship the old versions anyway.

aaronjensen · 2015-07-31T07:10:52Z

src/components/createConnectDecorator.js

-      static displayName = `Connector(${getDisplayName(DecoratedComponent)})`;
+const emptyBinder = () => ({});
+
+const identityMerge = (slice, actionsCreators, props) => ({...slice, ...actionsCreators, ...props});


We ran into some annoying bugs with a merge like this, specifically the fact that props can override slice. Given that Connect's primary responsibility is getting state from the store and passing it along, I'm not sure if allowing props to override it by default is right.

Some other options:

Put props first, allowing slice and actionCreators to override it.

Do not merge props in by default, a custom merge can be passed in to do this explicitly if desired.

For what it's worth, the specific problem we ran into involved nested connectors and the first connector splatting its props to the 2nd. They both had a loaded flag, but they meant different things. Nested connectors can happen especially with nested routes.

Also, it looks like props are splatted in again, below, in the actual render, but in that case they have lower precedence, which is good, I think.

I agree props should go first.

Also, if props go first here, they need to be removed from render.

I'm fine either way we will just need the eventual documentation to indicate that if you set a custom merge you will need to spread props for them to ever reach your underlying component.

either way this gives more control so that is good

Yes, I think that would be fine.

… component. provide wrapper method to get underlying ref to provide access to unerlying refs methods

aaronjensen · 2015-07-31T07:21:48Z

src/components/createConnectDecorator.js

+      componentDidMount() {
+        if (subscribing) {
+          this.subscribed = true;
+          this.unsubscribe = this.context.store.subscribe(::this.handleChange);


Are you planning to stick to ES6 on this repo too?

I think it's no big deal here because it's not a public facing example.

Not sure I understand the original comment? I believe the package builds to es5 for consumption by default. Are you referring to not using es7 features or something?

We're removing ES7 features from Redux examples just so people can work with something stable and don't assume Redux requires ES7. But using it in source is fine.

oh gotcha, object spread and all that.

…ctionCreators, remove from DecoratedComponent render

gnoff · 2015-07-31T07:48:52Z

@gaearon mind also checking out this additional PR (currently on my fork only since it looks cleaner comparing to my master)

gnoff#1

gnoff · 2015-07-31T07:52:28Z

test/components/connect.spec.js

+      let div = TestUtils.findRenderedDOMComponentWithTag(tree, 'div');
+      expect(div.props.children).toBe(42);
+
+      tree.setState({ select: selectB });


This test is failing. might just be a jsdom issue but I'm really unfamiliar with react unit testing so I'd love some eyes on this to either fix the test or confirm that this functionality is actually broken in my implementation

I don't think this test makes sense anymore. select isn't a prop now, you must specify it during the class definition.
I'm not sure why it breaks but you can remove it.

Alright, i'll remove it

Keats · 2015-07-31T08:58:46Z

src/components/createConnectDecorator.js

+  const { Component, PropTypes } = React;
+  const storeShape = createStoreShape(PropTypes);
+
+  return function connect(select, dispatchBinder = emptyBinder, mergeHandler = identityMerge) {


should we rename select to selector as mentioned in #15 ?

I'm in favor of arguments being named consistently as nouns or verbs. so select -> selector or change

selector <-> select
dispatchBinder <-> dispatchBind
mergeHandler <-> merge

quirinpa · 2015-07-31T10:20:20Z

Cool xD When do you think this will be merged? Can't wait to use it hehe

gaearon · 2015-07-31T10:25:10Z

About a week after Redux 1.0 is released, I presume.

quirinpa · 2015-07-31T10:26:08Z

ok, thank you @gaearon :)

exposing underlying component ref for access to component methods via DecoratedComponent#getUnderlyingRef

gnoff · 2015-07-31T17:18:19Z

Should we consider changing the static member DecoratedComponent on our wrapped component. If you use @connect()(Component) you will end up with the Symbol for Component pointing to the decorated component Component.DecoratedComponent pointing to the undecorated component. This feels... misleading?

Perhaps we change static DecoratedComponent in createConnectDecorator.js to static UndecoratedComponent

or static Decoratee

…ispatch and updated corresponding method names. this is to avoid confusion with the redux utility bindActionCreators and the fact that the second argument doesn not have to necessarily be related to action creators in any way

quirinpa · 2015-07-31T20:14:35Z

@Keats Hey Keats, great job! I have a question: If we use the merge method to select inside a store, does it update only when that particular "reselect" gives up different values or does it re-render every time the main store changes?

rt2zz · 2015-08-01T05:57:26Z

+1 just tried it out and it is working as described

rt2zz · 2015-08-01T23:31:31Z

note: nesting connectors does not work in react 0.13, but does work in 0.14. Something to do with the change to parent contexts.

Keats · 2015-08-02T18:03:47Z

@quirinpa I'm not the one that did the PR!
It's @gnoff

quirinpa · 2015-08-03T01:59:22Z

@Keats oh :P my mistake

fast forwarding

…patch

gnoff added 4 commits July 30, 2015 01:02

WIP connectWrapper that will eventually replace connectDecorator and …

f6ef06e

…Connector

intermediate commit. old decorator and connector are still present

063777a

implemented new connect api. takes in selectState, dispatchBinder, me…

ce8bf1e

…rge and puts sequential results of such on props of decorated component. Connector is intended to be deprecated. older connectDecorator is now called connecctDeprecated

forgot to add {...this.props} to DecoratedComponent in render

a788376

aaronjensen reviewed Jul 31, 2015
View reviewed changes

gaearon mentioned this pull request Jul 31, 2015

Start API change for connect decorator #15

Closed

use ref callback to place instance of underlying component on wrapped…

b446894

… component. provide wrapper method to get underlying ref to provide access to unerlying refs methods

aaronjensen reviewed Jul 31, 2015
View reviewed changes

gnoff added 3 commits July 31, 2015 00:24

remvoing deprecated code for Connector and original connectDecorator

df18c6d

move props in identityMerge to front to allow override by slice and a…

74c3d9d

…ctionCreators, remove from DecoratedComponent render

merging master

44287dc

gnoff reviewed Jul 31, 2015
View reviewed changes

gnoff mentioned this pull request Jul 31, 2015

Experimenting will exposing underlying component ref for access to component methods gnoff/react-redux#1

Merged

Keats reviewed Jul 31, 2015
View reviewed changes

This was referenced Jul 31, 2015

Update Docs #14

Merged

Alternative API proposals #1

Closed

Merge pull request #1 from gnoff/underlyingRef

80ebc01

exposing underlying component ref for access to component methods via DecoratedComponent#getUnderlyingRef

Josh Story added 2 commits July 31, 2015 10:36

remove test for changing select prop as it no longer applies to new api

105a2aa

gaearon mentioned this pull request Jul 31, 2015

Possible to use @connect functionality without wrapping it in a new component? #8

Closed

gaearon mentioned this pull request Aug 4, 2015

Q: Should ConnectorDecorator::shouldComponentUpdate consider changes in state slice? #22

Closed

gnoff mentioned this pull request Aug 6, 2015

actions and dispatch #23

Closed

gnoff and others added 2 commits August 5, 2015 21:45

Merge pull request #2 from gaearon/master

9256e9d

fast forwarding

fixing test reference to bindActionCreators which was renamed bindDis…

c54a8b8

…patch

ellbee mentioned this pull request Aug 6, 2015

Can't pass props in selectors reduxjs/reselect#20

Closed

This was referenced Aug 7, 2015

Do we need shallowEqualScalar? #2

Closed

New API #25

Merged

gaearon merged commit c54a8b8 into reduxjs:master Aug 7, 2015

emmenko mentioned this pull request Aug 11, 2015

Fix for binding props and context emmenko/redux-react-router-async-example#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New API for connectDecorator implementation #16

New API for connectDecorator implementation #16

gnoff commented Jul 31, 2015

gaearon commented Jul 31, 2015

aaronjensen Jul 31, 2015

gaearon Jul 31, 2015

gaearon Jul 31, 2015

gnoff Jul 31, 2015

gaearon Jul 31, 2015

aaronjensen Jul 31, 2015

gaearon Jul 31, 2015

gnoff Jul 31, 2015

gaearon Jul 31, 2015

gnoff Jul 31, 2015

gnoff commented Jul 31, 2015

gnoff Jul 31, 2015

gaearon Jul 31, 2015

gnoff Jul 31, 2015

Keats Jul 31, 2015

gnoff Jul 31, 2015

quirinpa commented Jul 31, 2015

gaearon commented Jul 31, 2015

quirinpa commented Jul 31, 2015

gnoff commented Jul 31, 2015

quirinpa commented Jul 31, 2015

rt2zz commented Aug 1, 2015

rt2zz commented Aug 1, 2015

Keats commented Aug 2, 2015

quirinpa commented Aug 3, 2015

New API for connectDecorator implementation #16

New API for connectDecorator implementation #16

Conversation

gnoff commented Jul 31, 2015

API

gaearon commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnoff commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quirinpa commented Jul 31, 2015

gaearon commented Jul 31, 2015

quirinpa commented Jul 31, 2015

gnoff commented Jul 31, 2015

quirinpa commented Jul 31, 2015

rt2zz commented Aug 1, 2015

rt2zz commented Aug 1, 2015

Keats commented Aug 2, 2015

quirinpa commented Aug 3, 2015