Desktop: Enable React 16.3’s strict mode to catch bad practices

[j-f1]

React 16.3 introduces a new strict mode, which is enabled by wrapping all or a portion of your components with the <React.StrictMode> component.

According to the docs, it:

• identifies usages of lifecycle methods that will be deprecated in React 17 (componentWillMount, componentWillReceiveProps, and componentWillUpdate; migration docs are here)
• warns about using ref="string" (there’s a new React.createRef() type now that we could use instead of ref functions)
• and runs constructor, render, the new getDerivedStateFromProps, and the first argument to setState (if it’s a function) twice to ensure that they’re idempotent

All of these changes only occur when running in dev mode, so behavior in production shouldn’t change.

What do you think about enabling this @desktop/core? (I can’t ping you since the team is private)

[shiftkey]

@j-f1 keep in mind we already have tslint-rules/reactProperLifecycleMethodsRule.ts for validating components lifecycle methods, which will need to be updated as part of upgrading to 16.3.

The ref="string" rule also feels like something we could do in TS, but I can't see any usages of that in Desktop so that's not a huge concern for me right now. If we can get that validation occurring when linting we could then look to move things to React.createRef() down the track as that seems simpler to setup.

The idempotent stuff does seem interesting too, and will be more helpful as async rendering starts to become available.

It'd be nice to avoid sprinkling this everywhere to enable it - it seems to be able to be defined anywhere in the app, so having it closer to the root would be preferred.

(I can’t ping you since the team is private)

Don't worry about pinging people too early in the discussion - the core team take turns with the "first responder" role (it's my turn this week) so we're all somewhat paying attention to incoming issues like this.

[shiftkey]

Revisiting this thread, there's a couple of issues I'd like to address before enabling this for development mode:

• a lot of components are using these old lifecycle functions, so it'll generate a lot of noise in the console until we migrate them to the new API
• these components aren't really covered by any tests beyond the integration test which isn't satisfactory to catch subtle changes - it'd be nice to use something lean like react-testing-library to capture the "before" behaviour of each component before we open it up to migrating

It's easy to find all mentions of componentWillMount, componentWillReceiveProps and componentWillUpdate in the codebase, but I'd like to craft up a list of refactoring issues that others can help out with so we can track this migration work - on top of everything else we need to worry about currently.

[shiftkey]

I've opened #5092 to get react-virtualized up to date, and #5093 to track upgrading react-transition-group. Once those are in we can look at components in the codebase and figure out that migration strategy.