Fluentui: React.StrictMode compliance

[Nimelrian]

Maintainer note: see this GitHub project for progress on strict mode work, which will be released with the @fluentui/react version 8 release.

---

Bug Report

• __Package version(s)__:
  
  
  
  • office-ui-fabric-react 5.83.0
  
  
  • react 16.3.1
  
  

Priorities and help requested (not applicable if asking question):

Are you willing to submit a PR to fix? No (Due to lack of time and temporary CLA difficulties)

Requested priority: High

Describe the issue:

Mounting Fabric components in a StrictMode React tree causes many warnings in the console, due to unsafe lifecycle methods which will be deprecated in a future 16.x release and removed completely with React 17. Warnings concerning string refs are also logged.

This is caused by e.g. the Fabric component, CommandBar, ActionButton, BaseButton, Breadcrumb, DialogBase, FocusZone, ... (lots and lots of components).

I didn't find any issue concerning this, so I thought I'd give a heads up about the issue, since this will have to be fixed eventually if React 17 is to be supported by Fabric.

---

UPDATE 10/24/19: Some but not all of these issues have been addressed. Please see https://github.com/OfficeDev/office-ui-fabric-react/issues/4613#issuecomment-546165430 and following comments for current status.

[dzearing]

@Nimelrian Thank you. We will absolutely be cleaning these up. Most of the string refs have been converted, but we have some leftovers.

I think the best way for us to address this is to turn on StrictMode in our ssr test suite, which will fail if any warnings are generated. Then make sure that passwes.

[Nimelrian]

Thanks for the confirmation that this is being handled already. I couldn't find any issue concerning the transition/refactoring/warnings so I thought it'd be a good idea to ask about it.

I'll see if I can be of any help for a smooth transition, if both, my time and employer allow for it.

[stale[bot]]

This issue has been automatically marked as stale because it has not activity for 30 days. It will be closed if no further activity occurs within 14 days of this comment. Thank you for your contributions to Fabric React!
Why am I receiving this notification?

[dzearing]

We will do this before the next major release. But @Nimelrian if you have cycles to help here, please submit prs! There are lots of small things to fix here.

[Nimelrian]

I'll see if I can get a local clone of fabric setup and do some work over the coming evenings. No promises though 😄

[cliffkoh]

@micahgodbolt @dzearing @aditima @Jahnp I think this work item needs to be tracked as a whole-team effort kind of like what we did with CSS in JS work and scheduled as a team-wide sprint. Just like with CSS in JS, this is too much for any one shield engineer to work on alone.

[cliffkoh]

@micahgodbolt is the right thing to create a new Project or Milestone to track this process for all the components?

[micahgodbolt]

Yeah, I like milestones for a set of defined work that will eventually be completed (like the css-in-js conversion).

[ecraig12345]

UPDATE 10/24/19--see later comments starting at https://github.com/OfficeDev/office-ui-fabric-react/issues/4613#issuecomment-546165430 for outstanding work

---

David asked me to take a look at this as part of prep for the 7.0 release. Breaking out the different categories of issues listed here... https://reactjs.org/docs/strict-mode.html

Rewrite or rename unsafe lifecycle methods

componentWillReceiveProps, componentWillMount, and componentWillUpdate are being deprecated because they're unsafe for use in async rendering, and new methods getDerivedStateFromProps and getSnapshotBeforeUpdate have been added to help cover similar use cases in a safer way.

First migration step here is to rename existing method implementations to the UNSAFE_* names (which at least according to the initial blog post will still exist in React 17) and add a lint rule banning the un-prefixed names.

Removing usage of the unsafe lifecycles will be done over time and in some cases require help from component owners. I'll work on some of the simpler componentWillReceiveProps => getDerivedStateFromProps conversions and file issues against the component owners for the rest of those conversions, and for moving away from componentWillMount and componentWillUpdate (only a few components use those two lifecycles, but the use cases are complex enough that I'm not comfortable removing them myself).

Remove string refs

This is done, and we have a lint rule prohibiting new string refs.

Remove deprecated findDOMNode usage

I'll have to do some more research on this, as I'm not quite clear on which usage is considered deprecated and why.

Remove unexpected side effects from render phase lifecycles

Render phase lifecycles (constructor, getDerivedStateFromProps, shouldComponentUpdate, render, setState updater functions, and the deprecated methods componentWillMount, componentWillReceiveProps, componentWillUpdate) which have side effects will cause issues in async rendering mode--see link for details. To help detect side effects, strict mode runs these methods twice. Removing usage of the deprecated componentWill____ lifecycles will resolve some of these issues. TBD if/how we want to address the rest.

Removing legacy context API usage

Strict mode warns on usage of the legacy context API since it will be removed in a future major version. Based on a search, we're using context for the following:

• ResizeGroup, Context utility (#7364)
• ScrollablePane (#7365)
• Sticky (#7366)
• Layer tests and example (#7367)
• ScrollContainer/VirtualizedList (experiments) (#7368)
• Form (experiments) (#7369)

[KevinTCoughlin]

It would be fantastic to have a component converted as an example. I say this as someone who's only reference is https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html. Additional reference links welcome!

I'm all for migrating and am glad the React team has provided what seems like a proper migration path. Curious how it'll shape our existing components. 🚢

[ecraig12345]

Discovered something unfortunate...until BaseComponent stops using componentWillReceiveProps, nothing extending BaseComponent can use getDerivedStateFromProps. React gives a console warning if the old and new lifecycles are used in combination, and it doesn't call the old lifecycles.

BaseComponent uses componentWillReceiveProps to update the componentRef. This example from the blog post seems the closest to the use case here and recommends moving the logic to componentDidUpdate. @dzearing / others, any thoughts on whether that sounds okay or not (or alternatives)? I'll do some testing but would really appreciate input from someone with more React experience.

Current:

  public componentWillReceiveProps(newProps: Readonly<TProps>, newContext: any): void {
    this._updateComponentRef(this.props, newProps);
  }

  protected _updateComponentRef(currentProps: IBaseProps, newProps: IBaseProps = {}): void {
    if (currentProps.componentRef !== newProps.componentRef) {
      this._setComponentRef(currentProps.componentRef, null);
      this._setComponentRef(newProps.componentRef, this);
    }
  }

Proposed:

  public componentDidUpdate(prevProps: TProps, prevState: TState): void {
    this._updateComponentRef(prevProps, this.props);
  }

[OneCyrus]

Inheritence like the BaseComponent is basically an anti-pattern in reactjs. The prefered way would be composition: https://reactjs.org/docs/composition-vs-inheritance.html

As react is moving away from classes to functions (hooks), the inheritence-model will most likely prevent the use of hooks. i would recommend to review the use of inheritence when some major changes for react-strict support are planned.

[KevinTCoughlin]

In my mind, the area that would require extensive testing with the deprecation / removal of BaseComponent is its async helper. Currently, it ensures that asynchronous operations are cleaned-up per Component during componentWillUnmount() preventing memory leaks and other issues related to timing.

[lijunle]

Will React hooks help on async scenario?

[dzearing]

While I would agree that subclassing is an anti-pattern for React components, the purpose of BaseComponent was to:

1. Make sure all native dom events and async stuff gets cleaned up automatically with component lifetime. (There are likely cleaner ways to do this than BaseComponent._events.) The design goal was that there'd be NO CHANCE someone hooking up an event would forget to unhook it on unmount. If React supported native eventing (like StencilJS does), this would likely be a non issue as React could handle unhooking events. But to refer to the link above regarding composition, we could possibly have react components which just hook up native events or setIntervals/setTimers. Weird maybe, but less weird than magical member variables.

2. Have some try/catch error handling in lifecycle events. This pre-dated event boundaries and even React handling exceptions well. This has all changed, so only reason 1 is valid.

React hooks move us completely away from classes, as state moves into functional hooks and view moves into stateless functions.

[dzearing]

It's possible we could move away from BaseComponent. We should consider that.

[ecraig12345]

To clarify, we've worked around the immediate BaseComponent issue in #7393 by removing its usage of componentWillReceiveProps. Moving away from BaseComponent is still something we could consider, but it would probably be best to have that discussion on a separate issue given that this one is already rather overloaded.

[msft-github-bot]

:tada:This issue was addressed in #8037, which has now been successfully released as [email protected].:tada:

Handy links:

• Release Notes
• Official NPM Release
• Test the release with Developer Demo Site

[unindented]

We're seeing this didn't get addressed with the release of Fabric 7. Is anybody actively working on it?

[ecraig12345]

Are there some particular fixes you're interested in that haven't been done yet? @jdhuntington made some fixes as part of Fabric 7.

[KevinTCoughlin]

Are there some particular fixes you're interested in that haven't been done yet? @jdhuntington made some fixes as part of Fabric 7.

List's changes had to be reverted due to runtime errors in dogfood.

https://github.com/OfficeDev/office-ui-fabric-react/issues/7690#issuecomment-470700884

[unindented]

@ecraig12345 we were looking forward to wrapping our whole app in StrictMode to get ready for AsyncMode, but in order to do that we need all components to be clean of deprecated lifecycle methods.

[Kesshi]

I found a warning that is raised from the Nav component. It would be nice if this could be fixed also.

Warning: A future version of React will block javascript: URLs as a security precaution. Use event handlers instead if you can. If you need to generate unsafe HTML try using dangerouslySetInnerHTML instead. React was passed "javascript:".

[ecraig12345]

@Kesshi Good catch--could you open a separate bug about that? It will be easier to track and get fixed more quickly that way.

[ecraig12345]

Deprecated method inventory

Terms used

WRP = componentWillReceiveProps
WM = componentWillMount
WU = componentWillUpdate

In notes below, "semi-controlled" means that the component currently updates its state in response to new props, but also allows internal updates apart from props. Moving the logic to getDerivedStateFromProps breaks these cases because the state updates are overwritten whenever props change. In some cases, this can be worked around with the following pattern (which is absolutely not recommended for use in new code):

// DO NOT use this in new code!
componentDidUpdate(prevProps: IProps, prevState: IState): void {
  if (prevProps !== this.props) {
    // move componentWillReceiveProps logic here
  }
}

Aside from generally being an anti-pattern, the temporary state/props mismatch could potentially cause issues, especially if any of the render methods have side effects. It also isn't a great idea for components where rendering is expensive (IIRC further state updates made in componentDidUpdate are made before changes from the update are committed to the DOM, but the component's render method still has to re-run). But it could be a workable temporary solution in certain cases.

OUFR

|Name|WRP|WM|WU|Notes|
|----|------------------|----------|-----------|-----|
|Autofill|y|||has special updateValueInWillReceiveProps prop…?|
|Calendar|y|||medium: use derived state but cache calculations|
|CalendarDay|y|||easy (use derived state)|
|CalloutContent||y|y|maybe hard|
|Coachmark|y|||maybe hard|
|PositioningContainer||y|y|maybe hard|
|ComboBox|y|||semi-controlled, props mutation side effects|
|ContextualMenu||y|y|maybe hard|
|DatePicker|y|||semi-controlled, side effects|
|DetailsList|y||y|inherently scary|
|DetailsRow|y|||basic semi-controlled|
|Dropdown|y|||semi-controlled, side effects|
|BaseExtendedPicker|y|||easy: just using state as a cache (use getters and prop access instead)|
|BaseFloatingPicker|y|||maybe hard|
|SuggestionsControl|y|||medium: side effects (could maybe move to didUpdate)|
|FocusTrapZone|y|||maybe hard|
|GroupedList|y|||maybe hard|
|GroupHeader|y|||basic semi-controlled|
|Image|y|||medium|
|List|y|||maybe hard|
|Modal|y|||maybe hard|
|OverflowSet|||y|maybe hard|
|PersonaCoin|y|||medium|
|BasePicker|y||y|maybe hard|
|Popup||y||easy change (move logic to constructor) but needs testing|
|ResizeGroup|y|||maybe hard|
|SearchBox|y|y||basic semi-controlled; cwm usage seems like typo (should be Unmount)|
|BaseSelectedItemsList|y||y|maybe hard|
|SpinButton|y|||maybe hard: semi-controlled, side effects, really odd logic|
|SwatchColorPicker|y|||basic semi-controlled|
|MaskedTextField|y|||maybe hard|

date-time

|Name|WRP|WM|WU|Notes|
|----|------------------|----------|-----------|-----|
|Calendar|y|||basic semi-controlled|
|CalendarDayGrid|y|||medium: use derived state but cache calculations|
|DatePicker|y|||semi-controlled, side effects|

experiments (low priority)

|Name|WRP|WM|WU|Notes|
|----|------------------|----------|-----------|-----|
|FloatingSuggestions|y|||maybe hard|
|SuggestionsControl|y|||medium: side effects (could maybe move to didUpdate)|
|Tile|y|||maybe hard|
|TilesList|y||y|medium: state used as cache, side effects|
|VirtualizedList|||y|maybe hard|

website/example-app-base

|Name|WRP|WM|WU|Notes|
|----|------------------|----------|-----------|-----|
|Site|y|||medium: side effects, state used as cache|
|LoadingComponent|y|||probably easy: move to constructor|

[ecraig12345]

Note that I'm not currently working on this, but wanted to share this info in case it's useful to others.

[ecraig12345]

Other categories of outstanding issues (which to reiterate, I'm not currently working on):

Remove unexpected side effects from render phase lifecycles

Background (copied from earlier comment): Render phase lifecycles (constructor, getDerivedStateFromProps, shouldComponentUpdate, render, setState updater functions, and the deprecated methods componentWillMount, componentWillReceiveProps, componentWillUpdate) which have side effects will cause issues in async rendering mode--see link for details. To help detect side effects, strict mode runs these methods twice.

We still have a significant number of components which use side effects in these lifecycles, but it's a lot harder to flag with a simple code search. So we unfortunately may have to address these as issues are reported.

Removing legacy context API usage

Only remaining usage is in experiments (therefore lower-priority) in ScrollContainer and VirtualizedList (#7368).

Remove deprecated findDOMNode usage

Still not sure which usage is deprecated and why.

[OneCyrus]

Note that I'm not currently working on this, but wanted to share this info in case it's useful to others.

is someone else working on this? for us it‘s one of the very last library which is blocking us from react concurrent.

[ecraig12345]

Unfortunately not right now. We have a lot of major work underway in other areas. Specifically we are focusing on making sure all of our components meet a high standards of accessibility, as well as carefully planning a roadmap that will make strong use of hooks and React best practices going forward.

[unindented]

Moving everything to hooks would also fix this.

[ecraig12345]

That's ideally the goal, but Fabric is a large library and changing all the components to be hooks-based will take time--especially doing it in a way that maintains decent functional parity and doesn't introduce too many API breaks (which is essential for many of our consumers).

[unindented]

The migration to FluentUI is going to be breaking anyways, so maybe that's the time to make those changes?

[MasterCassim]

Still many findings unfortunately :(

[ecraig12345]

We're still in the process of working on this--like I said previously, it will take time and has to be balanced with a lot of other priorities.

@MLoughry and others have started working on functional component migrations (while maintaining existing APIs) which will be released as part of @fluentui/react 8. You can see the work so far in our pre-release package @fluentui/react-next, though the components here are not yet stable and shouldn't be used in production (we'll announce something when it's closer to ready).

Converged Fluent UI components (combination of Fabric and @fluentui/react-northstar) will also be strict mode compliant, but I don't think we have a release date for that yet.

[ecraig12345]

We've been working on this--tracking on this board and probably planning on getting this done for version 8. https://github.com/microsoft/fluentui/projects/35

[ecraig12345]

12770 has just been updated with details on our version 8 plan and timelines. Strict mode compliance will be one of the major features.

[ecraig12345]

The obvious issues have been fixed in all version 8 components exported by default from @fluentui/react. It's possible there are still some more subtle problems related to render-phase side effects, so please file a new issue if you find anything like that. Version 8 is currently in beta, and you can try it out now by installing @fluentui/react@beta--see the release notes for more details.

There are still a few components under @fluentui/react-experiments (formerly @uifabric/experiments) which use UNSAFE_ methods, but we didn't prioritize fixing these due to their experimental status. If you're using one of those components and need strict/concurrent mode, please file a new issue and we can figure out an appropriate path forward.

[ecraig12345]

One clarification: most strict/concurrent mode fixes were made exclusively in version 8, and it's not feasible to port them back to version 7. This is because many of them (such as function component conversions) involve some level of subtle breaking changes which wouldn't be safe or feasible for some of our major partners to pick up except in a major release.

See the release notes and migration guide for more details on upgrading, including specifics of the potential breaking changes related to function component conversions.