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

React 16 experiment: rewrite React-Redux to use new context API #898

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 26 additions & 12 deletions src/components/Provider.js
@@ -1,8 +1,10 @@
import { Component, Children } from 'react'
import React, { Component, Children } from 'react'
import PropTypes from 'prop-types'
import { storeShape, subscriptionShape } from '../utils/PropTypes'
import { storeShape } from '../utils/PropTypes'
import warning from '../utils/warning'

import {ReactReduxContext} from "./context";

let didWarnAboutReceivingStore = false
function warnAboutReceivingStore() {
if (didWarnAboutReceivingStore) {
Expand All @@ -20,20 +22,35 @@ function warnAboutReceivingStore() {
}

export function createProvider(storeKey = 'store', subKey) {
const subscriptionKey = subKey || `${storeKey}Subscription`

class Provider extends Component {
getChildContext() {
return { [storeKey]: this[storeKey], [subscriptionKey]: null }
}

constructor(props, context) {
super(props, context)
this[storeKey] = props.store;

const {store} = props;

this.state = {
storeState : store.getState(),
dispatch : store.dispatch,
};
}

componentDidMount() {
const {store} = this.props;

// TODO What about any actions that might have been dispatched between ctor and cDM?
this.unsubscribe = store.subscribe( () => {
this.setState({storeState : store.getState()});
});
}

render() {
return Children.only(this.props.children)
return (
<ReactReduxContext.Provider value={this.state}>
{Children.only(this.props.children)}
</ReactReduxContext.Provider>
);
}
}

Expand All @@ -45,14 +62,11 @@ export function createProvider(storeKey = 'store', subKey) {
}
}


Provider.propTypes = {
store: storeShape.isRequired,
children: PropTypes.element.isRequired,
}
Provider.childContextTypes = {
[storeKey]: storeShape.isRequired,
[subscriptionKey]: subscriptionShape,
}

return Provider
}
Expand Down
167 changes: 68 additions & 99 deletions src/components/connectAdvanced.js
@@ -1,19 +1,19 @@
import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import { Component, createElement } from 'react'
import React, { Component, createElement } from 'react'

import Subscription from '../utils/Subscription'
import { storeShape, subscriptionShape } from '../utils/PropTypes'
import {ReactReduxContext} from "./context";
import { storeShape } from '../utils/PropTypes'

let hotReloadingVersion = 0
const dummyState = {}
function noop() {}
function makeSelectorStateful(sourceSelector, store) {
function makeSelectorStateful(sourceSelector) {
// wrap the selector in an object that tracks its results between runs.
const selector = {
run: function runComponentSelector(props) {
run: function runComponentSelector(props, storeState) {
try {
const nextProps = sourceSelector(store.getState(), props)
const nextProps = sourceSelector(storeState, props)
if (nextProps !== selector.props || selector.error) {
selector.shouldComponentUpdate = true
selector.props = nextProps
Expand Down Expand Up @@ -75,20 +75,12 @@ export default function connectAdvanced(
...connectOptions
} = {}
) {
const subscriptionKey = storeKey + 'Subscription'
const version = hotReloadingVersion++

const contextTypes = {
[storeKey]: storeShape,
[subscriptionKey]: subscriptionShape,
}
const childContextTypes = {
[subscriptionKey]: subscriptionShape,
}

return function wrapWithConnect(WrappedComponent) {
invariant(
typeof WrappedComponent == 'function',
typeof WrappedComponent === 'function',
`You must pass a component to the function returned by ` +
`${methodName}. Instead received ${JSON.stringify(WrappedComponent)}`
)
Expand All @@ -113,33 +105,25 @@ export default function connectAdvanced(
}

class Connect extends Component {
constructor(props, context) {
super(props, context)
constructor(props) {
super(props)

this.version = version
this.state = {}
this.renderCount = 0
this.store = props[storeKey] || context[storeKey]
this.propsMode = Boolean(props[storeKey])
this.storeState = null;


this.setWrappedInstance = this.setWrappedInstance.bind(this)
this.renderChild = this.renderChild.bind(this);

// TODO How do we express the invariant of needing a Provider when it's used in render()?
Copy link
Member

Choose a reason for hiding this comment

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

The Consumer of createContext will take on the default value when it's not under the paired Provider. So, I think just leaving this invariant in place would do.

Copy link
Member

Choose a reason for hiding this comment

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

/*
invariant(this.store,
`Could not find "${storeKey}" in either the context or props of ` +
`"${displayName}". Either wrap the root component in a <Provider>, ` +
`or explicitly pass "${storeKey}" as a prop to "${displayName}".`
)

this.initSelector()
this.initSubscription()
}

getChildContext() {
// If this component received store from props, its subscription should be transparent
// to any descendants receiving store+subscription from context; it passes along
// subscription passed to it. Otherwise, it shadows the parent subscription, which allows
// Connect to control ordering of notifications to flow top-down.
const subscription = this.propsMode ? null : this.subscription
return { [subscriptionKey]: subscription || this.context[subscriptionKey] }
*/
}

componentDidMount() {
Expand All @@ -151,24 +135,22 @@ export default function connectAdvanced(
// To handle the case where a child component may have triggered a state change by
// dispatching an action in its componentWillMount, we have to re-run the select and maybe
// re-render.
this.subscription.trySubscribe()
this.selector.run(this.props)
this.selector.run(this.props, this.storeState);
if (this.selector.shouldComponentUpdate) this.forceUpdate()
}

componentWillReceiveProps(nextProps) {
this.selector.run(nextProps)

UNSAFE_componentWillReceiveProps(nextProps) {
// TODO Do we still want/need to implement sCU / cWRP now?
this.selector.run(nextProps, this.storeState);
}

shouldComponentUpdate() {
return this.selector.shouldComponentUpdate
}


componentWillUnmount() {
if (this.subscription) this.subscription.tryUnsubscribe()
this.subscription = null
this.notifyNestedSubs = noop
this.store = null
this.selector.run = noop
this.selector.shouldComponentUpdate = false
}
Expand All @@ -185,85 +167,71 @@ export default function connectAdvanced(
this.wrappedInstance = ref
}

initSelector() {
const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions)
this.selector = makeSelectorStateful(sourceSelector, this.store)
this.selector.run(this.props)
}

initSubscription() {
if (!shouldHandleStateChanges) return

// parentSub's source should match where store came from: props vs. context. A component
// connected to the store via props shouldn't use subscription from context, or vice versa.
const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey]
this.subscription = new Subscription(this.store, parentSub, this.onStateChange.bind(this))

// `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in
// the middle of the notification loop, where `this.subscription` will then be null. An
// extra null check every change can be avoided by copying the method onto `this` and then
// replacing it with a no-op on unmount. This can probably be avoided if Subscription's
// listeners logic is changed to not call listeners that have been unsubscribed in the
// middle of the notification loop.
this.notifyNestedSubs = this.subscription.notifyNestedSubs.bind(this.subscription)
}

onStateChange() {
this.selector.run(this.props)

if (!this.selector.shouldComponentUpdate) {
this.notifyNestedSubs()
} else {
this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate
this.setState(dummyState)
}
}

notifyNestedSubsOnComponentDidUpdate() {
// `componentDidUpdate` is conditionally implemented when `onStateChange` determines it
// needs to notify nested subs. Once called, it unimplements itself until further state
// changes occur. Doing it this way vs having a permanent `componentDidUpdate` that does
// a boolean check every time avoids an extra method call most of the time, resulting
// in some perf boost.
this.componentDidUpdate = undefined
this.notifyNestedSubs()
}

isSubscribed() {
return Boolean(this.subscription) && this.subscription.isSubscribed()
initSelector(dispatch, storeState) {
const sourceSelector = selectorFactory(dispatch, selectorFactoryOptions)
this.selector = makeSelectorStateful(sourceSelector)
this.selector.run(this.props, storeState);
}

addExtraProps(props) {
if (!withRef && !renderCountProp && !(this.propsMode && this.subscription)) return props
if (!withRef && !renderCountProp) return props;

// make a shallow copy so that fields added don't leak to the original selector.
// this is especially important for 'ref' since that's a reference back to the component
// instance. a singleton memoized selector would then be holding a reference to the
// instance, preventing the instance from being garbage collected, and that would be bad
const withExtras = { ...props }
if (withRef) withExtras.ref = this.setWrappedInstance
if (renderCountProp) withExtras[renderCountProp] = this.renderCount++
if (this.propsMode && this.subscription) withExtras[subscriptionKey] = this.subscription

return withExtras
}

render() {
const selector = this.selector
selector.shouldComponentUpdate = false
renderChild(providerValue) {
const {storeState, dispatch} = providerValue;

if (selector.error) {
throw selector.error
} else {
return createElement(WrappedComponent, this.addExtraProps(selector.props))
}
this.storeState = storeState;

if(this.selector) {
this.selector.run(this.props, storeState);
}
else {
this.initSelector(dispatch, storeState);
}

if (this.selector.error) {
// TODO This will unmount the whole tree now that we're throwing in render. Good idea?

Choose a reason for hiding this comment

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

It could be... But, I would think may need to be accompanied by a componentDidCatch to gracefully handle the Error.

Choose a reason for hiding this comment

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

Shouldn't the user be responsible to handle the errors?

// TODO Related: https://github.com/reactjs/react-redux/issues/802
throw this.selector.error
}
else if(this.selector.shouldComponentUpdate) {
//console.log(`Re-rendering component (${displayName})`, this.selector.props);
this.selector.shouldComponentUpdate = false;
this.renderedElement = createElement(WrappedComponent, this.addExtraProps(this.selector.props));
}
else {
//console.log(`Returning existing render result (${displayName})`, this.props)
}

return this.renderedElement;
}

render() {
return (
<ReactReduxContext.Consumer>
{this.renderChild}
</ReactReduxContext.Consumer>
)
}
}

Connect.WrappedComponent = WrappedComponent
Connect.displayName = displayName
Connect.childContextTypes = childContextTypes
Connect.contextTypes = contextTypes
Connect.propTypes = contextTypes
// TODO We're losing the ability to add a store as a prop. Not sure there's anything we can do about that.
//Connect.propTypes = contextTypes

// TODO With connect no longer managing subscriptions, I _think_ is is all unneeded
/*
if (process.env.NODE_ENV !== 'production') {
Connect.prototype.componentWillUpdate = function componentWillUpdate() {
// We are hot reloading!
Expand All @@ -290,6 +258,7 @@ export default function connectAdvanced(
}
}
}
*/

return hoistStatics(Connect, WrappedComponent)
}
Expand Down
3 changes: 3 additions & 0 deletions src/components/context.js
@@ -0,0 +1,3 @@
import React from "react";

export const ReactReduxContext = React.createContext(null);
7 changes: 0 additions & 7 deletions src/utils/PropTypes.js
@@ -1,12 +1,5 @@
import PropTypes from 'prop-types'

export const subscriptionShape = PropTypes.shape({
trySubscribe: PropTypes.func.isRequired,
tryUnsubscribe: PropTypes.func.isRequired,
notifyNestedSubs: PropTypes.func.isRequired,
isSubscribed: PropTypes.func.isRequired,
})

export const storeShape = PropTypes.shape({
subscribe: PropTypes.func.isRequired,
dispatch: PropTypes.func.isRequired,
Expand Down