Mattermost-server: Remove deprecated lifecycle methods from channel list

Created on 24 Sep 2019  路  13Comments  路  Source: mattermost/mattermost-server

The following React lifecycle methods have been deprecated by React and will start to be removed starting in React 17:

  • componentWillMount
  • componentWillReceiveProps
  • componentWillUpdate

The React team has published a guide discussing the changes and how to migrate to the new lifecycle methods here: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

As part of a campaign to remove these methods from https://github.com/mattermost/mattermost-mobile, these methods should be replaced in the following files:

  • app/screens/create_channel/create_channel.js
  • app/screens/channel_peek/channel_peek.js
  • app/components/sidebars/main/channels_list/list/list.js
  • app/components/sidebars/main/channels_list/filtered_list/filtered*list.js
  • app/screens/more_channels/more_channels.js
  • app/components/team_icon/team_icon.js

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-18764

AreTechnical Debt Medium Hacktoberfest Help Wanted TecReact Native

All 13 comments

can I work on this?

Thank you @sowmiyamuthuraman!

Hey @sowmiyamuthuraman,

How is your work going? Can we help with anything?

yeah! I am working on it. I need some help regarding when to use getDerivedStateFromProps and componentDidMount lifecycle methods.

Would you mind posing your question here so we can help?

Hi canni work on this?

@a-c-sreedhar-reddy sure! Thank you :)

@jasonblais There is an eslint rule which does not allow useage of setState in componentDidUpdate. Can I disable that for this component because we had to update the state when the props change.

I think in list component instead of storing the sections in the state better way would be to calculate it in render.

I'm not sure - @hmhealey would you know the answer to the question from @a-c-sreedhar-reddy?

@a-c-sreedhar-reddy Ideally, we'd avoid setState in componentDidUpdate since that causes the component to render twice in a row, and we'd either remove the state or use something like deriveStateFromProps instead, but there will be cases where the extra re-render is acceptable to avoid complicating the rest of the code. I'll let you know in your PR if there's anywhere that I think we could get rid of it. In that case, you can use an eslint-disable-line on the setState call since we'd still like to discourage that pattern by default

@hmhealey

I could have removed sections from state and memoed it in the render method. But I did not because sections state is used in componentDidUpdate.

I could have used getDerivedStateFromProps to compute sections state. But then sections state would be computed even if one unrelated prop changes eg(favoriteChannelIds).
buildSections result could not be memoed or it is not possible to know which props have updated since getDerivedStateFromProps is static.

So I went with componentDidUpdate.

I also felt it would be easy to refactor this component to a functional component in the future if componentDidUpdate is used which would be equivalent to useEffect.

Yeah, that should be fine. I used to be more concerned with avoiding the extra render, but it makes everything much more complicated in cases like this when we're wanting to recalculate a complex value when a certain prop changes. You could theoretically also store the prop in state since that lets you compare with the previous value in getDerivedStateFromProps, but that's a lot of extra logic which can add other issues.

Was this page helpful?
0 / 5 - 0 ratings