Skip to content
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

New API for connectDecorator implementation #16

Merged
merged 13 commits into from
Aug 7, 2015
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/components/createAll.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import createProvider from './createProvider';
import createProvideDecorator from './createProvideDecorator';

import createConnector from './createConnector';
import createConnectDecoratorDeprecated from './createConnectDecoratorDeprecated';
import createConnectDecorator from './createConnectDecorator';

export default function createAll(React) {
Expand All @@ -11,7 +12,8 @@ export default function createAll(React) {

// Higher-order components (decorators)
const provide = createProvideDecorator(React, Provider);
const connect = createConnectDecorator(React, Connector);
const connectDeprecated = createConnectDecoratorDeprecated(React, Connector);
const connect = createConnectDecorator(React);

return { Provider, Connector, provide, connect };
return { Provider, Connector, provide, connect, connectDeprecated };
}
121 changes: 109 additions & 12 deletions src/components/createConnectDecorator.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,121 @@
import createStoreShape from '../utils/createStoreShape';
import getDisplayName from '../utils/getDisplayName';
import shallowEqualScalar from '../utils/shallowEqualScalar';
import shallowEqual from '../utils/shallowEqual';
import isPlainObject from '../utils/isPlainObject';
import wrapActionCreators from '../utils/wrapActionCreators';
import invariant from 'invariant';

export default function createConnectDecorator(React, Connector) {
const { Component } = React;
const emptySelector = () => ({});

return function connect(select) {
return DecoratedComponent => class ConnectorDecorator extends Component {
static displayName = `Connector(${getDisplayName(DecoratedComponent)})`;
const emptyBinder = () => ({});

const identityMerge = (slice, actionsCreators, props) => ({...slice, ...actionsCreators, ...props});

Choose a reason for hiding this comment

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

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:

  1. Put props first, allowing slice and actionCreators to override it.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree props should go first.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that would be fine.



export default function createConnectDecorator(React) {
const { Component, PropTypes } = React;
const storeShape = createStoreShape(PropTypes);

return function connect(select, dispatchBinder = emptyBinder, mergeHandler = identityMerge) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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


const subscribing = select ? true : false;
const selectState = select || emptySelector;
const bindActionCreators = isPlainObject(dispatchBinder) ? wrapActionCreators(dispatchBinder) : dispatchBinder;
Copy link

Choose a reason for hiding this comment

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

Using the same name as a redux function but with a different signature is a bit confusing.
Also @gaearon I don't really see the point of the wrapActionCreators mentioned in #1, why not just pass the actionCreators directly to connect and bind it using redux.bindActionCreators (at line 93 in this file) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is really unfortunate, especially since we have an invariant check that reports this method name if you pass in a bad dispatchBind. I would love to change this

Regarding the wrap, I want to keep this since the second arg is not actually a 'definitely wrap action creators' function, it's a general purpose opportunity to do something with dispatch. the wrap is just convenience for the overhwhelming number of times the use case will call for a simple wrap+bind

Copy link

Choose a reason for hiding this comment

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

but wouldn't it be simpler to just differentiate functions (ie you want to do something with dispatch) and plain object (ie you want to bindActionCreators).
The connector in the second case would be

@connect(
  null,
  myActionCreators
)

and in the first case:

@connect(
null,
(dispatch) => (..)
)

which will be bound automatically by the connector
Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, this PR supports the two scenarios above. if connect receives a plain object for the dispatchBinder argument it assumes it is plain action creators and internally wraps them in wrapActionCreators (which in turn calls bindActionCreators from redux)

If it is not a plain object it needs to be a function that returns and object (presumably using the dispatch arg it is handed... but you can really do whatever you want here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed bindActionCreators internal to createConnectDecorator to instead be bindDispatch.

This maps to the public api more directly and avoids name collision confusion with redux/bindActionCreators

const merge = mergeHandler;
Copy link

Choose a reason for hiding this comment

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

I don't understand the point of that one, it would be actually easier to keep it called mergeHandler to distinguish easily from the Component.merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per lint rules I can't reassign the function arguments so it needs a different name if we want an opportunity to do anything with the argument (which with default args we aren't at the moment) then it will need a different name.

I actually don't mind that these const values overload the naming of the local component methods since it implies a delegating type relationship (which is exactly how they are used). I'd prefer to leave these alone but will take additional input


return DecoratedComponent => class ConnectDecorator extends Component {
static displayName = `ConnectDecorator(${getDisplayName(DecoratedComponent)})`;
static DecoratedComponent = DecoratedComponent;

shouldComponentUpdate(nextProps) {
return !shallowEqualScalar(this.props, nextProps);
static contextTypes = {
store: storeShape.isRequired
};

shouldComponentUpdate(nextProps, nextState) {
return (this.subscribed && !this.isSliceEqual(this.state.slice, nextState.slice)) ||
!shallowEqualScalar(this.props, nextProps);
}

render() {
return (
<Connector select={state => select(state, this.props)}>
{stuff => <DecoratedComponent {...stuff} {...this.props} />}
</Connector>
isSliceEqual(slice, nextSlice) {
const isRefEqual = slice === nextSlice;
if (isRefEqual) {
return true;
} else if (typeof slice !== 'object' || typeof nextSlice !== 'object') {
return isRefEqual;
}
Copy link

Choose a reason for hiding this comment

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

if (isRefEqual || (typeof slice !== 'object' || typeof nextSlice !== 'object') {
  return isRefEqual;
}

would be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was lifted straight from Connector

@gaearon is this a micro optimization already?

return shallowEqual(slice, nextSlice);
}

constructor(props, context) {
super(props, context);
this.state = {
...this.selectState(props, context),
...this.bindActionCreators(context)
};
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh gotcha, object spread and all that.

}
}

componentWillUnmount() {
if (subscribing) {
this.unsubscribe();
}
}

handleChange(props = this.props) {
const nextState = this.selectState(props, this.context);
if (!this.isSliceEqual(this.state.slice, nextState.slice)) {
this.setState(nextState);
}
}

selectState(props = this.props, context = this.context) {
const state = context.store.getState();
const slice = selectState(state);

Choose a reason for hiding this comment

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

We have found use for selectState to take props as its second argument. It'd be nice if that is supported.

Choose a reason for hiding this comment

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

@gaearon in the other thread you said:

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.

I haven't read it all out, but I think it's just not quite as efficient. Specifically, we could avoid a setState here: https://github.com/gaearon/react-redux/pull/16/files#diff-71fbb22950d38c66596e5c31a9bf35eaR74 if the subset we care about (based on props) did not change but the superset did.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something preventing you from using merge argument for this? That's the intended use case: unlike select, it executes on every render, so you can access props there. Otherwise we'll have to re-evalute select on every prop change.

Choose a reason for hiding this comment

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

No, the merge argument works. I didn't realize that we could avoid running select on props changes.

That said, select is probably something that is very fast, and w/ the current shouldComponentUpdate above, it'd trigger renders at times when nothing has changed in my scenario, so I'm not sure where the line is. Maybe it's just weird that our container uses props to determine a state subset, but it seems like it may be a common thing for reusable containers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a common thing, but then it's weird you can't do the same for actions. And you can't because that would be a perf problem (functions being bound every time). At least merge is a single place to go when you want to do something with props. I think it's OK.


invariant(
isPlainObject(slice),
'The return value of `select` prop must be an object. Instead received %s.',
slice
);

return { slice };
}

bindActionCreators(context = this.context) {
const { dispatch } = context.store;
const actionCreators = bindActionCreators(dispatch);

invariant(
isPlainObject(actionCreators),
'The return value of `bindActionCreators` prop must be an object. Instead received %s.',
actionCreators
);

return { actionCreators };
}

merge(props = this.props, state = this.state) {
const { slice, actionCreators } = state;
const merged = merge(slice, actionCreators, props);

invariant(
isPlainObject(merged),
'The return value of `merge` prop must be an object. Instead received %s.',
merged
);

return merged;
}

render() {
return <DecoratedComponent {...this.props} {...this.merge()} />;
}
};
};
Expand Down
25 changes: 25 additions & 0 deletions src/components/createConnectDecoratorDeprecated.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import getDisplayName from '../utils/getDisplayName';
import shallowEqualScalar from '../utils/shallowEqualScalar';

export default function createConnectDecorator(React, Connector) {
const { Component } = React;

return function connect(select) {
return DecoratedComponent => class ConnectorDecorator extends Component {
static displayName = `Connector(${getDisplayName(DecoratedComponent)})`;
static DecoratedComponent = DecoratedComponent;

shouldComponentUpdate(nextProps) {
return !shallowEqualScalar(this.props, nextProps);
}

render() {
return (
<Connector select={state => select(state, this.props)}>
{stuff => <DecoratedComponent {...stuff} {...this.props} />}
</Connector>
);
}
};
};
}
2 changes: 1 addition & 1 deletion src/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import createAll from './components/createAll';

export const { Provider, Connector, provide, connect } = createAll(React);
export const { Provider, Connector, provide, connect, connectDeprecated } = createAll(React);
Copy link

Choose a reason for hiding this comment

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

@gaearon should it still export Connector and connectDeprecated rather than removing it completely ?
edit: noticed your comment above

2 changes: 1 addition & 1 deletion src/native.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react-native';
import createAll from './components/createAll';

export const { Provider, Connector, provide, connect } = createAll(React);
export const { Provider, Connector, provide, connect, connectDeprecated } = createAll(React);
5 changes: 5 additions & 0 deletions src/utils/wrapActionCreators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { bindActionCreators } from 'redux';

export default function wrapActionCreators(actionCreators) {
return dispatch => bindActionCreators(actionCreators, dispatch);
}
16 changes: 8 additions & 8 deletions test/components/Connector.spec.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import expect from 'expect';
import jsdomReact from './jsdomReact';
import React, { PropTypes, Component } from 'react/addons';
import { createStore } from 'redux';
import { createStore, combineReducers } from 'redux';
import { Connector } from '../../src/index';

const { TestUtils } = React.addons;
Expand Down Expand Up @@ -32,7 +32,7 @@ describe('React', () => {
}

it('should receive the store in the context', () => {
const store = createStore({});
const store = createStore(() => ({}));

const tree = TestUtils.renderIntoDocument(
<Provider store={store}>
Expand Down Expand Up @@ -74,7 +74,7 @@ describe('React', () => {
const subscribe = store.subscribe;

// Keep track of unsubscribe by wrapping subscribe()
const spy = expect.createSpy(() => {});
const spy = expect.createSpy(() => ({}));
store.subscribe = (listener) => {
const unsubscribe = subscribe(listener);
return () => {
Expand All @@ -101,7 +101,7 @@ describe('React', () => {

it('should shallowly compare the selected state to prevent unnecessary updates', () => {
const store = createStore(stringBuilder);
const spy = expect.createSpy(() => {});
const spy = expect.createSpy(() => ({}));
function render({ string }) {
spy();
return <div string={string}/>;
Expand Down Expand Up @@ -129,10 +129,10 @@ describe('React', () => {
});

it('should recompute the state slice when the select prop changes', () => {
const store = createStore({
const store = createStore(combineReducers({
a: () => 42,
b: () => 72
});
}));

function selectA(state) {
return { result: state.a };
Expand Down Expand Up @@ -174,7 +174,7 @@ describe('React', () => {
});

it('should pass dispatch() to the child function', () => {
const store = createStore({});
const store = createStore(() => ({}));

const tree = TestUtils.renderIntoDocument(
<Provider store={store}>
Expand All @@ -191,7 +191,7 @@ describe('React', () => {
});

it('should throw an error if select returns anything but a plain object', () => {
const store = createStore({});
const store = createStore(() => ({}));

expect(() => {
TestUtils.renderIntoDocument(
Expand Down
4 changes: 2 additions & 2 deletions test/components/Provider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('React', () => {
}

it('should add the store to the child context', () => {
const store = createStore({});
const store = createStore(() => ({}));

const tree = TestUtils.renderIntoDocument(
<Provider store={store}>
Expand All @@ -36,7 +36,7 @@ describe('React', () => {
it('should replace just the reducer when receiving a new store in props', () => {
const store1 = createStore((state = 10) => state + 1);
const store2 = createStore((state = 10) => state * 2);
const spy = expect.createSpy(() => {});
const spy = expect.createSpy(() => ({}));

class ProviderContainer extends Component {
state = { store: store1 };
Expand Down
Loading