Skip to content

Commit

Permalink
performance optimizations: HEADS UP API change too
Browse files Browse the repository at this point in the history 
* React.forwardRef is VERY slow, on the order of 2x slower in our benchmark. So we only enable it if withRef="forwardRef" folks using withRef=true will get an error telling them to update and not rely on getWrappedInstance() but just to use the ref directly
* renderCountProp is removed, as this is natively supported in React dev tools now
* all usages of shallowEquals are removed for pure components, it was unnecessary.
* instead of allowing passing in a custom Context consumer in props, it is now required to be passed in via connect options at declaration time.
  • Loading branch information
@cellog
cellog committed Aug 19, 2018
1 parent 68c1ffa commit 3c222f2
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 68 deletions.
108 changes: 71 additions & 37 deletions src/components/connectAdvanced.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import React, { Component, PureComponent } from 'react'
import propTypes from 'prop-types'
import shallowEqual from 'shallow-equals'
import { isValidElementType } from 'react-is'

import Context from './Context'

const Consumer = Context.Consumer
const ReduxConsumer = Context.Consumer

export default function connectAdvanced(
/*
Expand Down Expand Up @@ -52,14 +51,14 @@ export default function connectAdvanced(
withRef = false,

// the context consumer to use
consumer = Consumer,
consumer = ReduxConsumer,

// additional options are passed through to the selectorFactory
...connectOptions
} = {}
) {
invariant(!withRef,
`withRef is removed. To access the wrapped instance, simply pass in ref`
invariant(renderCountProp === undefined,
`renderCountProp is removed. render counting is built into the latest React dev tools profiling extension`
)

invariant(storeKey === 'store',
Expand All @@ -69,26 +68,39 @@ export default function connectAdvanced(
'ConnectedComponent consumer={context.Consumer} /></Provider>'
)

const Consumer = consumer

return function wrapWithConnect(WrappedComponent) {
invariant(
isValidElementType(WrappedComponent),
`You must pass a component to the function returned by ` +
`${methodName}. Instead received ${JSON.stringify(WrappedComponent)}`
)
invariant(!withRef || withRef === 'forwardRef',
'withRef must be set to the text "forwardRef." Reference uses React.forwardRef and you may now access ref ' +
`directly instead of using getWrappedInstance() in component ${wrappedComponentName}`
)

const wrappedComponentName = WrappedComponent.displayName
|| WrappedComponent.name
|| 'Component'

const displayName = getDisplayName(wrappedComponentName)

class PureWrapper extends PureComponent {
class PureWrapper extends Component {
shouldComponentUpdate(nextProps) {
return nextProps.derivedProps !== this.props.derivedProps
}

render() {
const { forwardRef, ...props } = this.props
return <WrappedComponent {...props} ref={forwardRef} />
let { forwardRef, derivedProps } = this.props
return <WrappedComponent {...derivedProps} ref={forwardRef} />
}
}

PureWrapper.propTypes = {
count: propTypes.object,
derivedProps: propTypes.object,
forwardRef: propTypes.oneOfType([
propTypes.func,
propTypes.object
Expand All @@ -108,19 +120,31 @@ export default function connectAdvanced(
WrappedComponent
}

class Connect extends Component {
const OuterBase = connectOptions.pure ? PureComponent : Component

class Connect extends OuterBase {
constructor(props) {
super(props)
invariant(!props[storeKey],
'Passing redux store in props has been removed and does not do anything. ' +
'To use a custom redux store for a single component, ' +
'create a custom React context with React.createContext() and pass the Provider to react-redux\'s provider ' +
'and the Consumer to this component as in <Provider context={context.Provider}><' +
wrappedComponentName + ' consumer={context.Consumer} /></Provider>'
)
if (withRef) {
invariant(!props.props[storeKey],
'Passing redux store in props has been removed and does not do anything. ' +
'To use a custom redux store for a single component, ' +
'create a custom React context with React.createContext() and pass the Provider to react-redux\'s provider ' +
'and the Consumer to this component as in <Provider context={context.Provider}><' +
wrappedComponentName + ' consumer={context.Consumer} /></Provider>'
)
} else {
invariant(!props[storeKey],
'Passing redux store in props has been removed and does not do anything. ' +
'To use a custom redux store for a single component, ' +
'create a custom React context with React.createContext() and pass the Provider to react-redux\'s provider ' +
'and the Consumer to this component as in <Provider context={context.Provider}><' +
wrappedComponentName + ' consumer={context.Consumer} /></Provider>'
)

}
this.memoizeDerivedProps = this.makeMemoizer()
this.renderWrappedComponent = this.renderWrappedComponent.bind(this)
this.renderCount = 0
}

makeMemoizer() {
Expand All @@ -132,7 +156,7 @@ export default function connectAdvanced(
let called = false
return (state, props, store) => {
if (called) {
const sameProps = connectOptions.pure && shallowEqual(lastProps, props)
const sameProps = connectOptions.pure && lastProps === props
const sameState = lastState === state
if (sameProps && sameState) {
return lastDerivedProps
Expand All @@ -146,7 +170,7 @@ export default function connectAdvanced(
lastProps = props
lastState = state
const nextProps = sourceSelector(state, props)
if (shallowEqual(lastDerivedProps, nextProps)) {
if (lastDerivedProps === nextProps) {
return lastDerivedProps
}
lastDerivedProps = nextProps
Expand All @@ -156,46 +180,56 @@ export default function connectAdvanced(

renderWrappedComponent(value) {
invariant(value,
`Could not find "store" in either the context of ` +
`Could not find "store" in the context of ` +
`"${displayName}". Either wrap the root component in a <Provider>, ` +
`or pass a custom React context provider to <Provider> and the corresponding ` +
`React context consumer to ${displayName}.`
`React context consumer to ${displayName} in connect options.`
)
const { state, store } = value
const { forwardRef, ...otherProps } = this.props
let derivedProps = this.memoizeDerivedProps(state, otherProps, store)
if (withRef) {
const { forwardRef, props } = this.props
let derivedProps = this.memoizeDerivedProps(state, props, store)
if (connectOptions.pure) {
return <PureWrapper derivedProps={derivedProps} forwardRef={forwardRef} />
}

if (renderCountProp) {
derivedProps = { ...derivedProps, [renderCountProp]: this.renderCount++ }
return <WrappedComponent {...derivedProps} ref={forwardRef} />
}
let derivedProps = this.memoizeDerivedProps(state, this.props, store)
if (connectOptions.pure) {
return <PureWrapper {...derivedProps} forwardRef={forwardRef}/>
return <PureWrapper derivedProps={derivedProps} />
}
return <WrappedComponent {...derivedProps} ref={forwardRef} />

return <WrappedComponent {...derivedProps} />
}

render() {
const MyConsumer = this.props.consumer || consumer
return (
<MyConsumer>
<Consumer>
{this.renderWrappedComponent}
</MyConsumer>
</Consumer>
)
}
}

Connect.WrappedComponent = WrappedComponent
Connect.displayName = displayName
Connect.propTypes = {
consumer: propTypes.object,
forwardRef: propTypes.oneOfType([
propTypes.func,
propTypes.object
])
if (withRef) {
Connect.propTypes = {
props: propTypes.object,
forwardRef: propTypes.oneOfType([
propTypes.func,
propTypes.object
])
}
}

if (!withRef) {
return hoistStatics(Connect, WrappedComponent)
}

function forwardRef(props, ref) {
return <Connect {...props} forwardRef={ref} />
return <Connect props={props} forwardRef={ref} />
}

const forwarded = React.forwardRef(forwardRef)
Expand Down
53 changes: 22 additions & 31 deletions test/components/connect.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@ describe('React', () => {
{makeContainer(() => ({}), () => ({}), () => 1)}
</ProviderMock>
)
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledTimes(2)
expect(spy.mock.calls[0][0]).toMatch(
/mergeProps\(\) in Connect\(Container\) must return a plain object/
)
Expand All @@ -1293,7 +1293,7 @@ describe('React', () => {
{makeContainer(() => ({}), () => ({}), () => 'hey')}
</ProviderMock>
)
expect(spy).toHaveBeenCalledTimes(1)
expect(spy).toHaveBeenCalledTimes(2)
expect(spy.mock.calls[0][0]).toMatch(
/mergeProps\(\) in Connect\(Container\) must return a plain object/
)
Expand Down Expand Up @@ -1390,7 +1390,7 @@ describe('React', () => {
const decorator = connect(state => {
actualState = state
return {}
})
}, undefined, undefined, { consumer: context.Consumer })
const Decorated = decorator(Container)
const mockStore = {
dispatch: () => {},
Expand Down Expand Up @@ -1468,7 +1468,7 @@ describe('React', () => {
}
}

const decorator = connect(state => state, null, null)
const decorator = connect(state => state, undefined, undefined, { withRef: 'forwardRef' })
const Decorated = decorator(Container)

const ref = React.createRef()
Expand Down Expand Up @@ -2174,19 +2174,19 @@ describe('React', () => {
const store2 = createStore((state = 0, action) => (action.type === 'INC' ? state + 1 : state))
const customContext = React.createContext()

@connect(state => ({ count: state }))
@connect(state => ({ count: state }), undefined, undefined, { consumer: customContext.Consumer })
class A extends Component {
render() { return <B consumer={customContext.Consumer} /> }
render() { return <B /> }
}

const mapStateToPropsB = jest.fn(state => ({ count: state }))
@connect(mapStateToPropsB)
@connect(mapStateToPropsB, undefined, undefined, { consumer: customContext.Consumer })
class B extends Component {
render() { return <C {...this.props} /> }
}

const mapStateToPropsC = jest.fn(state => ({ count: state }))
@connect(mapStateToPropsC)
@connect(mapStateToPropsC, undefined, undefined, { consumer: customContext.Consumer })
class C extends Component {
render() { return <D /> }
}
Expand Down Expand Up @@ -2244,6 +2244,16 @@ describe('React', () => {
expect(spy).not.toHaveBeenCalled()
})

it('should error on withRef=true (must be "forwardRef")', () => {
class Container extends Component {
render() {
return <div>hi</div>
}
}
expect(() => connect(undefined, undefined, undefined, { withRef: true })(Container))
.toThrow(/withRef must be set/)
})

it('should error on receiving a custom store key', () => {
const connectOptions = { storeKey: 'customStoreKey' }

Expand All @@ -2258,17 +2268,6 @@ describe('React', () => {
}).toThrow(/storeKey has been removed/)
})

it('should error on withRef', () => {
function Container() {
return <div>hi</div>
}
expect(() => {
connect(undefined, undefined, undefined, { withRef: true })(Container)
}).toThrow(
'withRef is removed. To access the wrapped instance, simply pass in ref'
)
})

it('should error on custom store', () => {
function Comp() {
return <div>hi</div>
Expand All @@ -2282,21 +2281,13 @@ describe('React', () => {
}).toThrow(/Passing redux store/)
})

it('should add a renderCount prop if specified in connect options', () => {
let value = 0
const store = createStore(() => ({ value: value++}))
it('should error on renderCount prop if specified in connect options', () => {
function Comp(props) {
return <div>{props.count}</div>
}
const Container = connect(undefined,undefined,undefined,{ renderCountProp: 'count' })(Comp)
const tester = rtl.render(
<ProviderMock store={store}>
<Container/>
</ProviderMock>
)
expect(tester.queryByText('0')).not.toBe(null)
store.dispatch({ type: 'hi' })
expect(tester.queryByText('1')).not.toBe(null)
expect(() => {
connect(undefined,undefined,undefined,{ renderCountProp: 'count' })(Comp)
}).toThrow(/renderCountProp is removed/)
})
})
})

0 comments on commit 3c222f2

Please sign in to comment.