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

Remove usages of async-unsafe lifecycle methods #919

Merged
merged 9 commits into from
Jun 21, 2018
Merged
Show file tree
Hide file tree
Changes from 5 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
2 changes: 1 addition & 1 deletion docs/api.md
Expand Up @@ -377,7 +377,7 @@ It does not modify the component class passed to it; instead, it *returns* a new

* [`renderCountProp`] *(String)*: if defined, a property named this value will be added to the props passed to the wrapped component. Its value will be the number of times the component has been rendered, which can be useful for tracking down unnecessary re-renders. Default value: `undefined`

* [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render on `componentWillReceiveProps`. Default value: `true`
* [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render when parent component re-renders. Default value: `true`

* [`storeKey`] *(String)*: the key of props/context to get the store. You probably only need this if you are in the inadvisable position of having multiple stores. Default value: `'store'`

Expand Down
5 changes: 5 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Expand Up @@ -53,7 +53,8 @@
"lodash": "^4.17.5",
"lodash-es": "^4.17.5",
"loose-envify": "^1.1.0",
"prop-types": "^15.6.1"
"prop-types": "^15.6.1",
"react-lifecycles-compat": "^1.1.4"
Copy link

Choose a reason for hiding this comment

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

Why do you need the polyfill when you changed all the deprecated functions?

Copy link
Member

Choose a reason for hiding this comment

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

He's using getDerivedStateFromProps.

Copy link

Choose a reason for hiding this comment

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

I am using that too without a polyfill. No more warnings (in my own code).

Copy link
Member

Choose a reason for hiding this comment

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

It's for use with older versions of React. It's not to deal with deprecation warnings, it's to polyfill those lifecycle methods for React <= 16.2.

Copy link

Choose a reason for hiding this comment

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

Ah right, didn't think it that way around.
That brings me to a question for my own lib. What if I want to support the new and old Context API I guess I could just check if React.createContext is a function and then use it if avail?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pke : there's a distinct difference in how you declare and use the old and new context APIs. See my PR in #898 for an example.

We may need to do that check for React.createContext so we can use a polyfill like create-react-context, but the old and new context APIs aren't interchangeable.

Copy link
Contributor

Choose a reason for hiding this comment

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

New context API is probably going to require breaking changes to React Redux's public API for supporting multiple stores, making it a major version bump.

},
"devDependencies": {
"babel-cli": "^6.26.0",
Expand Down
4 changes: 2 additions & 2 deletions src/components/Provider.js
Expand Up @@ -38,8 +38,8 @@ export function createProvider(storeKey = 'store', subKey) {
}

if (process.env.NODE_ENV !== 'production') {
Provider.prototype.componentWillReceiveProps = function (nextProps) {
if (this[storeKey] !== nextProps.store) {
Provider.prototype.componentDidUpdate = function () {
if (this[storeKey] !== this.props.store) {
warnAboutReceivingStore()
}
}
Expand Down
52 changes: 32 additions & 20 deletions src/components/connectAdvanced.js
@@ -1,6 +1,7 @@
import hoistStatics from 'hoist-non-react-statics'
import invariant from 'invariant'
import { Component, createElement } from 'react'
import polyfill from 'react-lifecycles-compat'

import Subscription from '../utils/Subscription'
import { storeShape, subscriptionShape } from '../utils/PropTypes'
Expand All @@ -23,6 +24,10 @@ function makeSelectorStateful(sourceSelector, store) {
selector.shouldComponentUpdate = true
selector.error = error
}
},
clean: function cleanComponentSelector() {
selector.run = noop
selector.shouldComponentUpdate = false
}
}

Expand Down Expand Up @@ -86,6 +91,11 @@ export default function connectAdvanced(
[subscriptionKey]: subscriptionShape,
}

function getDerivedStateFromProps(nextProps, prevState) {
prevState.selector.run(nextProps)
Copy link
Contributor

@gaearon gaearon Apr 6, 2018

Choose a reason for hiding this comment

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

This looks mutative. What does it actually do?

I'm worried that this won't be safe and then avoiding the warning just gives a false sense of security.

The method signature is intentionally "get" to indicate it should be pure.

Copy link
Contributor Author

@Hypnosphi Hypnosphi Apr 6, 2018

Choose a reason for hiding this comment

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

It's not pure but it's idempotent: when you run in twice, it has the same effect as if it was just once.

I can try to make it pure by moving all the fields from selector to state though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that. It required some minor changes in tests: a7ba298#diff-b5fd7a42ea1b8efa382416b4a323003c

It's quite clear why setState calls count changed, but as for mapStateToPropsCalls, I'm not so confident

return null
}

return function wrapWithConnect(WrappedComponent) {
invariant(
typeof WrappedComponent == 'function',
Expand Down Expand Up @@ -117,7 +127,6 @@ export default function connectAdvanced(
super(props, context)

this.version = version
this.state = {}
this.renderCount = 0
this.store = props[storeKey] || context[storeKey]
this.propsMode = Boolean(props[storeKey])
Expand All @@ -129,7 +138,9 @@ export default function connectAdvanced(
`or explicitly pass "${storeKey}" as a prop to "${displayName}".`
)

this.initSelector()
this.state = {
selector: this.createSelector()
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at what StrictMode warns about yet. Does it complain about instance variables? Is that why we're putting the selector into state?

Copy link
Contributor Author

@Hypnosphi Hypnosphi Apr 5, 2018

Choose a reason for hiding this comment

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

No it doesn't.

This is because getDerivedStateFromProps is static, so one can't access instances from it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, I see it now. Got it.

}
this.initSubscription()
}

Expand All @@ -152,25 +163,20 @@ export default function connectAdvanced(
// 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)
if (this.selector.shouldComponentUpdate) this.forceUpdate()
}

componentWillReceiveProps(nextProps) {
this.selector.run(nextProps)
this.state.selector.run(this.props)
if (this.state.selector.shouldComponentUpdate) this.forceUpdate()
}

shouldComponentUpdate() {
return this.selector.shouldComponentUpdate
shouldComponentUpdate(nextProps, nextState) {
return nextState.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
this.state.selector.clean()
}

getWrappedInstance() {
Expand All @@ -185,10 +191,9 @@ export default function connectAdvanced(
this.wrappedInstance = ref
}

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

initSubscription() {
Expand All @@ -209,9 +214,9 @@ export default function connectAdvanced(
}

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

if (!this.selector.shouldComponentUpdate) {
if (!this.state.selector.shouldComponentUpdate) {
this.notifyNestedSubs()
} else {
this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate
Expand Down Expand Up @@ -247,7 +252,8 @@ export default function connectAdvanced(
}

render() {
const selector = this.selector
const selector = this.state.selector

selector.shouldComponentUpdate = false

if (selector.error) {
Expand All @@ -263,13 +269,13 @@ export default function connectAdvanced(
Connect.childContextTypes = childContextTypes
Connect.contextTypes = contextTypes
Connect.propTypes = contextTypes
Connect.getDerivedStateFromProps = getDerivedStateFromProps

if (process.env.NODE_ENV !== 'production') {
Connect.prototype.componentWillUpdate = function componentWillUpdate() {
Connect.prototype.componentDidUpdate = function componentDidUpdate() {
// We are hot reloading!
if (this.version !== version) {
this.version = version
this.initSelector()

// If any connected descendants don't hot reload (and resubscribe in the process), their
// listeners will be lost when we unsubscribe. Unfortunately, by copying over all
Expand All @@ -287,10 +293,16 @@ export default function connectAdvanced(
this.subscription.trySubscribe()
oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener))
}

const selector = this.createSelector()
selector.run(this.props)
this.setState({selector})
}
}
}

polyfill(Connect)

return hoistStatics(Connect, WrappedComponent)
}
}
15 changes: 15 additions & 0 deletions test/components/Provider.spec.js
Expand Up @@ -220,4 +220,19 @@ describe('React', () => {
store.dispatch({ type: 'APPEND', body: 'd' })
expect(childMapStateInvokes).toBe(4)
})

it('works in <StrictMode> without warnings', () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => {})
const store = createStore(() => ({}))

TestRenderer.create(
<React.StrictMode>
<Provider store={store}>
<div />
</Provider>
</React.StrictMode>
)

expect(spy).not.toHaveBeenCalled()
})
})
50 changes: 34 additions & 16 deletions test/components/connect.spec.js
Expand Up @@ -200,7 +200,7 @@ describe('React', () => {

@connect(state => ({ string: state }) )
class Container extends Component {
componentWillMount() {
componentDidMount() {
store.dispatch({ type: 'APPEND', body: 'a' })
}

Expand Down Expand Up @@ -944,8 +944,8 @@ describe('React', () => {

@connect((state) => ({ state }))
class Child extends Component {
componentWillReceiveProps(nextProps) {
if (nextProps.state === 'A') {
componentDidMount() {
if (this.props.state === 'A') {
store.dispatch({ type: 'APPEND', body: 'B' });
}
}
Expand Down Expand Up @@ -1218,14 +1218,13 @@ describe('React', () => {
const store = createStore(() => ({}))

function makeContainer(mapState, mapDispatch, mergeProps) {
return React.createElement(
@connect(mapState, mapDispatch, mergeProps)
class Container extends Component {
render() {
return <Passthrough />
}
@connect(mapState, mapDispatch, mergeProps)
class Container extends Component {
render() {
return <Passthrough />
}
)
}
return React.createElement(Container)
}

function AwesomeMap() { }
Expand Down Expand Up @@ -1928,7 +1927,7 @@ describe('React', () => {

@connect(mapStateFactory)
class Container extends Component {
componentWillUpdate() {
componentDidUpdate() {
updatedCount++
}
render() {
Expand Down Expand Up @@ -2009,7 +2008,7 @@ describe('React', () => {

@connect(null, mapDispatchFactory, mergeParentDispatch)
class Passthrough extends Component {
componentWillUpdate() {
componentDIdUpdate() {
Copy link

@pke pke Apr 5, 2018

Choose a reason for hiding this comment

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

There is a typo here? componentDIdUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix & add test

Copy link
Member

Choose a reason for hiding this comment

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

Just commited a fix for you.

updatedCount++
}
render() {
Expand Down Expand Up @@ -2121,10 +2120,6 @@ describe('React', () => {

@connect(null)
class Parent extends React.Component {
componentWillMount() {
this.props.dispatch({ type: 'fetch' })
}

componentWillUnmount() {
this.props.dispatch({ type: 'clean' })
}
Expand All @@ -2144,6 +2139,7 @@ describe('React', () => {
}

const store = createStore(reducer)
store.dispatch({ type: 'fetch' })
const div = document.createElement('div')
ReactDOM.render(
<ProviderMock store={store}>
Expand Down Expand Up @@ -2324,5 +2320,27 @@ describe('React', () => {
expect(mapStateToPropsC).toHaveBeenCalledTimes(2)
expect(mapStateToPropsD).toHaveBeenCalledTimes(2)
})

it('works in <StrictMode> without warnings', () => {
const spy = jest.spyOn(console, 'error').mockImplementation(() => {})
const store = createStore(stringBuilder)

@connect(state => ({ string: state }) )
class Container extends Component {
render() {
return <Passthrough {...this.props}/>
}
}

TestRenderer.create(
<React.StrictMode>
<ProviderMock store={store}>
<Container />
</ProviderMock>
</React.StrictMode>
)

expect(spy).not.toHaveBeenCalled()
})
})
})