Downshift: [hooks]: display component update warning - React 16.13

[loiacon]

• downshift version: 5.0.3

What you did:
Updated React version to 16.13.0

What happened:
When React was updated to 16.13.0, all components that have downshift show an error:
Warning: Cannot update a component from inside the function body of a different component.

Reproduction repository:
https://codesandbox.io/s/downshift-hook-react-1613-ufdnj

[silviuaavram]

can you take a look on what the issue is? is it in the hooks? in the component?

[loiacon]

I made some research and appear that's something when calling the callOnChangeProps function inside useEnhancedReducer

https://github.com/downshift-js/downshift/blob/5f40d453e1d2a6dfb81e99382713221638bf3de1/src/hooks/utils.js#L54-L89

[silviuaavram]

Ok we also have another issue related to how this is called. I will follow up with a fix soon.

https://github.com/downshift-js/downshift/issues/874

[brandonparis]

Looks like this is already being handled for onSelectedItemChange but I wanted to jump in and say I'm seeing the same problem with onInputValueChange. The codesandbox @loiacon provided above does a good job detailing the problem - add the onChange handler to onInputValueChange and that is my situation.

[silviuaavram]

I have this item in my backlog. We tried to fix it previously but we broke some controlled props logic. I will try a fix and reference it to this issue.

[silviuaavram]

Duplicate of https://github.com/downshift-js/downshift/issues/874. Will close that one.

[silviuaavram]

So, this is an unfortunate side effect in useEnhancedReducer when I callOnChangeProps.

I don't see how this is fixable at the moment. I need to call the onChange handlers before returning the reduced value, as I need to guarantee the handlers being called before re-render.

When building this I was inspired by Kent's article and received help from @layershifter. Maybe @kentcdodds may have an idea how to call those handlers without the side effect. Otherwise I am out of ideas :(

https://github.com/downshift-js/downshift/blob/5f40d453e1d2a6dfb81e99382713221638bf3de1/src/hooks/utils.js#L75-L95

[kentcdodds]

Is it possible to call the onChange handlers in a useEffect instead? Does it have to be called before a rerender?

[silviuaavram]

It is, but then we need 2 re-renders for the cases when users will have a controlled prop scenario. One re-render for our reducer to compute the new state, and another one after the controlled props get updated after the onChange call props.

I wanted to try to avoid this scenario.

[kentcdodds]

I'm not sure, but maybe this could help for the controlled props bit: https://github.com/kentcdodds/advanced-react-patterns/blob/bbb15b7cf08845ee048642df56f256ee3ff6066c/src/final/06.extra-1.js#L12-L37

[layershifter]

Ideal world 🌍

I would like to start from the point _how it should work_?

For simple components where we don't have any reducers and complicated state management the flow looks like:

render()
// =>
click()
changeState() & call on*() callback
// =>
apply the state that user passed
render()

Here is a sample codesandbox (please check logs on now.sh), there is a weird double rendering on CodeSandbox 😨

https://codesandbox.io/s/practical-haze-13wmw
https://csb-13wmw-ifbjhrz5k.now.sh/

useEnhancedReducer

I would say that the current implementation breaks the expected flow as callOnChangeProps occurs during rendering:

render()
// =>
click()
changeState() & call on*() callback
// =>
render()
render()

(React 16.12)

https://codesandbox.io/s/vigilant-wilbur-qvn92
https://csb-qvn92-2iri0y0ng.now.sh/

Solution? 🤔

My proposal is to modify dispatch to call handlers as it's the same behavior as with setState():

https://codesandbox.io/s/fancy-meadow-k2bn8
https://csb-k2bn8-1c4hf0uvp.now.sh/

[silviuaavram]

I am experimenting with the useState implementation above and it seems to work! Awesome @layershifter, this is genius! @kentcdodds you may be interested to look into that one as well, it might be useful to write about it, as it seems to be a great official way to handle this custom case (own reducer combined with user reducer combined with controlled props). Hell, I am inclined to write it myself, it's also a great initiative for me to try creating my personal blog website and an some initial post on it :D

Edit: might have some issues with passing updated props, as the state is not updated accordingly. It might need a small tweak since having a complete de-sync between state and controlled props might cause unexpected results when calling onChange handlers for instance, since those are still going to be called based on our state suggestions, and if the state is not updated correctly when props are updated, we might be in trouble.

maybe something like:

export function useEnhancedReducer(reducer, initialState, props) {
  const [uncontrolledState, setState] = useState(initialState)
  const state = getState(uncontrolledState, props)

  const dispatch = action => {

    const {stateReducer: stateReducerFromProps} = action.props
    const changes = reducer(state, action)
    const newState = stateReducerFromProps(state, {...action, changes})

    callOnChangeProps(action, state, newState)

    setState(newState)
  }

  const dispatchWithProps = action => dispatch({props, ...action})

  return [getState(state, props), dispatchWithProps]
}

[silviuaavram]

I will do more testing in the following days, this seems promising. Will create a PR and update/write some unit tests as well. Thanks again for helping out everyone! I am really happy we can move this forward finally.

[silviuaavram]

:tada: This issue has been resolved in version 5.2.1 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: