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 all 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: 2 additions & 4 deletions src/components/createAll.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
import createProvider from './createProvider';
import createProvideDecorator from './createProvideDecorator';

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

export default function createAll(React) {
// Wrapper components
const Provider = createProvider(React);
const Connector = createConnector(React);

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

return { Provider, Connector, provide, connect };
return { Provider, provide, connect };
}
125 changes: 113 additions & 12 deletions src/components/createConnectDecorator.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,125 @@
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) => ({ ...props, ...slice, ...actionsCreators});


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 bindDispatch = isPlainObject(dispatchBinder) ? wrapActionCreators(dispatchBinder) : dispatchBinder;
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.bindDispatch(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 };
}

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

invariant(
isPlainObject(actionCreators),
'The return value of `bindDispatch` 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;
}

getUnderlyingRef() {
return this.underlyingRef;
}

render() {
return <DecoratedComponent ref={component => (this.underlyingRef = component)} {...this.merge()} />;
}
};
};
Expand Down
88 changes: 0 additions & 88 deletions src/components/createConnector.js

This file was deleted.

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);
}
Loading