Downshift: React.StrictMode issue

[oliviertassinari]

• downshift version: 1.31.2

Relevant code or config: https://codesandbox.io/s/9lrw4n774w

What you did: I have added React.StrictMode.

What happened: a TypeError stateToSet is not a function error is thrown.

Notice that it works fine without React.StrictMode. It's potentially a React bug.

[stereobooster]

Warning: Unsafe lifecycle methods were found within a strict-mode tree:
    in IntegrationDownshift (created by WithStyles(IntegrationDownshift))
    in WithStyles(IntegrationDownshift)

componentWillReceiveProps: Please update the following components to use static getDerivedStateFromProps instead: Input, WithStyles(FormControl), WithStyles(Input)

componentWillUpdate: Please update the following components to use componentDidUpdate instead: Input

[oliviertassinari]

Ok, I have a new reproduction example without Material-UI as a dependency: https://codesandbox.io/s/9lrw4n774w.

• focus the input
• type a
• blur the input
• TypeError stateToSet is not a function

[kentcdodds]

Thanks! If someone would like to use the polyfill provided by the React team and update this component to not have this error in strict mode that would be wonderful!

[janhoogeveen]

@kentcdodds I'll take a stab at it!

Update: Here's my WIP: I still have to test the fork to see if it solves the problem, and I need to increase code coverage.

https://github.com/paypal/downshift/pull/410

[drobannx]

I noticed a similar issue with Downshift when using React.StrictMode. I debugged some and found what I think is going on.

When turning on strict mode, react says the following:

Strict mode can’t automatically detect side effects for you, but it can help you spot them
by making them a little more deterministic. This is done by intentionally double-invoking
the following methods:

• Class component constructor method
• The render method
• setState updater functions (the first argument)
• The static getDerivedStateFromProps lifecycle
  What is happening is the internalSetState function is doing the following:
  var isStateToSetFunction = typeof stateToSet === 'function'; outside the closure that is returned.

This is problematic because later this is invoked:
stateToSet = isStateToSetFunction ? stateToSet(state) : stateToSet;

The stateToSet was originally a function and works great the first time but when invoked a subsequent time due to the use of React.StrictMode, stateToSet is now an object but isStateToSetFunction remains true due to the closure.

It looks like the stateToSet reference gets updated with the newState instead of a new reference being created.

The main issue here is since the setState updater function is called twice with StrictMode on which is breaking the setState call that is returned in internalSetState.

I potentially see a couple fixes with my naive understanding of the internals:
1) Make the updater function pure in that it doesn't mutate any references in the closure. (not sure of the side effects of doing this, if any)
2) Move the isStateToSetFunction assignment down inside the setState updated function.

To me, number 2 seems to be more of a band-aid rather than fixing the root issue which I think number 1 would resolve.

[kentcdodds]

What if instead of isStateToSetFunction we just inlined the typeof stateToSet === 'function' wherever it's needed. I think that should work...

[drobannx]

@kentcdodds - We could do that but I think that only resolves one of the issues that StrictMode tries to alert about. In the React docs they have this about the first argument to the setState updater function:

prevState is a reference to the previous state. It should not be directly mutated

I created a quick PR #423 with some changes that I tested locally and seems to resolve the issue.

[kentcdodds]

I think you're mistaken. The code currently does not mutate the state (or prevState as it's referred to in the docs) object at all. It simply reassigns the variable binding which is different.

[drobannx]

Wouldn't creating a new object in getState and assigning that back to state change its reference? Maybe I am using the wrong terms here and confusing things. I guess prevState isn't being directly mutated but rather its reference changed which I was trying to prevent, maybe thats not a big deal.

[kentcdodds]

Changing a reference is not the same as mutating the value :+1: So no, it's not a big deal. One could argue it makes the code harder to follow, but as far as mutation is concerned, your code doesn't do anything to change that. The getState function actually creates a new object entirely so we're safe from mutation :)

[drobannx]

Sorry for the confusion with the terms I was using and thanks for the explanation. I can remove that change from the PR as that change did not impact the cause of the error in StrictMode.

[franklixuefei]

Is this still a problem?

[drobannx]

@franklixuefei - I believe this issue was resolved with https://github.com/paypal/downshift/pull/423