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
2 changes: 1 addition & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
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
4 changes: 2 additions & 2 deletions src/components/Provider.js
Original file line number Diff line number Diff line change
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
17 changes: 11 additions & 6 deletions src/components/connectAdvanced.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,10 @@ export default function connectAdvanced(
if (this.selector.shouldComponentUpdate) this.forceUpdate()
}

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

shouldComponentUpdate() {
shouldComponentUpdate(nextProps) {
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a hack with bad side effects. Can we use getSnapshotBeforeUpdate instead?

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 try that. Is it called before sCU?

Copy link
Contributor Author

@Hypnosphi Hypnosphi Apr 3, 2018

Choose a reason for hiding this comment

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

Looks like it's not: https://reactjs.org/docs/react-component.html#updating

I can try to move selector from field on instance to field on state, then it will be available in static getDerivedStateFromProps

if (nextProps !== this.props) {
this.selector.run(nextProps)
}
return this.selector.shouldComponentUpdate
}

Expand Down Expand Up @@ -248,6 +247,10 @@ export default function connectAdvanced(

render() {
const selector = this.selector

// Handle forceUpdate
if (!selector.shouldComponentUpdate) selector.run(this.props)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be handled by sCU above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sCU doesn't run when forceUpdating


selector.shouldComponentUpdate = false

if (selector.error) {
Expand All @@ -265,7 +268,7 @@ export default function connectAdvanced(
Connect.propTypes = contextTypes

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
Expand All @@ -287,6 +290,8 @@ export default function connectAdvanced(
this.subscription.trySubscribe()
oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener))
}

if (this.selector.shouldComponentUpdate) this.setState(dummyState)
}
}
}
Expand Down
15 changes: 15 additions & 0 deletions test/components/Provider.spec.js
Original file line number Diff line number Diff line change
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()
})
})
47 changes: 34 additions & 13 deletions test/components/connect.spec.js
Original file line number Diff line number Diff line change
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,7 +2120,7 @@ describe('React', () => {

@connect(null)
class Parent extends React.Component {
componentWillMount() {
UNSAFE_componentWillMount() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried to change it to componentDidMount it broke the test. Maybe it should be changed to constructor, I'm not sure

Copy link
Member

Choose a reason for hiding this comment

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

What error did you get with cDM? The cWM function is not consequential here anyways; this is testing for unmount cleanups. So, I'd be fine with removing it and putting a dispatch outside of the ReactDOM.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.

data was null

this.props.dispatch({ type: 'fetch' })
}

Expand Down Expand Up @@ -2324,5 +2323,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()
})
})
})