Skip to content

Conversation

@ranneyd
Copy link
Contributor

@ranneyd ranneyd commented Sep 27, 2019

componentWillReceiveProps is going to be deprecated, and is giving console warnings to anyone using this lib.

Since the usage just caused a side-effect and didn't modify the state, we can just replace it with componentDidUpdate and get the same effect.

Here's a good example:
https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#side-effects-on-props-change

if (prevProps.disabled !== disabled) {
if (disabled) {
this.removeEventListeners();
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this change cause the mousedown event listener to be added after the DOM is updated? ie, could this change cause a user click to be missed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not. The events are independent of React, so adding/removing them doesn't depend on the stage of the component's lifecycle. Additionally, as you can see in the code snippet below (the code that gets called by this function) it applies the listener to document, which is independent of the component, so it doesn't care what state the component is in. In other words, clicking outside (anywhere in the document) is totally independent of the component; it needs to change when props change because the props might change to enable/disable it. Whether that happens before or after the render is irrelevant.

  addMouseDownEventListener(useCapture) {
    this.removeMouseDown = addEventListener(
      document,
      'mousedown',
      this.onMouseDown,
      { capture: useCapture },
    );
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that's convincing.

@ljharb ljharb changed the title Replaceing componentWillReceiveProps with componentDidUpdate Replace componentWillReceiveProps with componentDidUpdate Sep 27, 2019
@ljharb ljharb added the semver-patch Fixes, refactors, etc. label Sep 27, 2019
@ljharb ljharb merged commit 0b5befd into airbnb:master Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Fixes, refactors, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants