react-sortablejs icon indicating copy to clipboard operation
react-sortablejs copied to clipboard

Warning: Cannot update a component from inside the function body of a different component.

Open digilist opened this issue 5 years ago • 18 comments

Describe the bug The warning Warning: Cannot update a component from inside the function body of a different component. appears with the latest React version 16.13 when using the example provided in the readme file.

To Reproduce Steps to reproduce the behavior: Create a new component using the example code with React v16.13.0

Expected behavior No warning :)

Information This happens because the setList method is passed to the child component which results now in a warning.

More information in the react release notes: https://reactjs.org/blog/2020/02/26/react-v16.13.0.html

Versions react-sortable = ^2.0.11 react = ^16.13.0

digilist avatar Mar 05 '20 16:03 digilist

This is because ReactSortable is compiled into a function that explicitly calls

props.setList(newList, _this.sortable, store);

If this was source code, wrapping this call with useEffect would fix the issue:

useEffect(() => { props.setList(newList, _this.sortable, store); }, []);

Bad news is that according to https://github.com/facebook/react/issues/18178#issuecomment-592662192 this error may break the whole ReactSortable component in the nearest future...

swba avatar Mar 13 '20 21:03 swba

After some digging, this comment describes the issue well: https://github.com/facebook/react/issues/18178#issuecomment-602323184

This happens because the setList method is passed to the child component which results now in a warning.

@digilist I don't see anywhere in the code where setList is passed to a child component. Would you be able to elaborate on this?

It looks like the error may be coming from this LOC, within the constructor:

https://github.com/SortableJS/react-sortablejs/blob/5ec354869e95187137e9b3121dc68cf0b5d72217/src/react-sortable.tsx#L56

I hypothesize that we'll need to remove this from the constructor (because this may be during rendering), wait for the component to render, then call it somewhere else (componentDidUpdate, but only after first render).

If this was source code, wrapping this call with useEffect would fix the issue:

This component does not use hooks thus far. I'm actually trying to port this to hooks because it'll be easier to deal fix issues with.

waynevanson avatar Apr 01 '20 00:04 waynevanson

@waynevanson sorry, I think I didn't express it well. I think it's coming from the callback that is passed via the setList parameter as it is used in the example here: https://github.com/SortableJS/react-sortablejs#function-component

digilist avatar Apr 01 '20 07:04 digilist

Also running into this warning on ^16.13.0.

stevecastaneda avatar Apr 04 '20 20:04 stevecastaneda

I also got this is, but it was the list property, not setList. If you use useState or useSelector as your list you get this warning. If I do a "cheap" clone with JSON.parse(JSON.stringify(listFromStateOrSelector)) the warning disappears.

hgeldenhuys avatar Apr 07 '20 05:04 hgeldenhuys

@hgeldenhuys I was able to confirm that fixed it here, but oddly enough, when I reverted back to using the state directly, the error was gone. 🤔

stevecastaneda avatar Apr 07 '20 14:04 stevecastaneda

I'm not quire understanding how to replicate the issue. Would someone be able to put this into a codesandbox? I want to make sure I'm not just changing code for the hell of it.

waynevanson avatar Apr 08 '20 05:04 waynevanson

Probably the warning is gone with the latest react versions. In my application I do not see the warning anymore 🤔

digilist avatar Apr 08 '20 07:04 digilist

@digilist I agree, now on 16.13.1 and not seeing the error.

https://github.com/facebook/react/releases/tag/v16.13.1

Likely due to this?

Revert warning for cross-component updates that happen inside class render lifecycles (componentWillReceiveProps, shouldComponentUpdate, and so on). (@gaearon in #18330)

stevecastaneda avatar Apr 08 '20 14:04 stevecastaneda

Yeah.

It still pointed out a legit problem but we’ve silenced it for classes because it’s “too late” to fix there. People tend to treat constructors as places to do side effects in some legacy code.

gaearon avatar Apr 09 '20 02:04 gaearon

I'll still address the issue. It's becoming obvious that setlist shouldn't be called within the constructor

waynevanson avatar Apr 09 '20 07:04 waynevanson

@waynevanson if I can help you to refactor to hooks and functional components, just let me know. :)

https://codesandbox.io/s/affectionate-blackburn-7et9h?file=/src/App.js

webdesign2be avatar May 13 '20 01:05 webdesign2be

I also got this is, but it was the list property, not setList. If you use useState or useSelector as your list you get this warning. If I do a "cheap" clone with JSON.parse(JSON.stringify(listFromStateOrSelector)) the warning disappears.

I tried working with a copy of the list as you say but it still doesn't update the component inner state after change index

eppisapia-legacy avatar May 29 '20 16:05 eppisapia-legacy

I'm not quire understanding how to replicate the issue. Would someone be able to put this into a codesandbox? I want to make sure I'm not just changing code for the hell of it.

There is a sandbox with issue https://codesandbox.io/s/intelligent-paper-iodho?file=/src/App.js

eppisapia-legacy avatar May 29 '20 16:05 eppisapia-legacy

Maybe not the best solution, but adding a setTimeout solves it for me for now.

<ReactSortable
  list={items}
  setList={(nextItems) => setTimeout(() => setItems(nextItems))}
>
  {items.map((item) => (
    <Item key={item.id} {...item} />
  ))}
</ReactSortable>

https://codesandbox.io/s/sweet-varahamihira-zdb19

siwonia avatar Aug 18 '20 07:08 siwonia

will be fixed in #175 by adding hooks

waynevanson avatar Sep 27 '20 14:09 waynevanson

@waynevanson Do you know when that update #175 will be set to release? I'm encountering this exact same issue and we are working through a final QA pass before we noticed this issue surface.

I would love to stay with this library since it's been the most straightforward to integrate into our builds.

nakedgremlin avatar Oct 16 '20 04:10 nakedgremlin

Maybe this would help someone. I just left setList empty and used onEnd handler instead, and called a state update from it. Inside the reducer, I handle the elements s

 <ReactSortable
      list={optionsVal}
      onEnd={(evt, sortable, store) => {
        if (evt.oldIndex === undefined || evt.newIndex === undefined) {
          return;
         }
        dispatch({ type: "OPTION_MOVED", from: evt.oldIndex, to: evt.newIndex });
      }}
      setList={() => {
         // just because it is required
       }}              
     >...list...</ReactSortable>

Catching that in the reducer, I handle swapping manually:

moveItem(options, fromIndex, toIndex);

export function moveItem<T>(items: T[], from: number, to: number): T[] {
  const moveResult = [...items];
  const moved = items[from];
  moveResult.splice(from, 1);
  moveResult.splice(to, 0, moved);
  return moveResult;
}

velosipedist avatar Nov 07 '20 02:11 velosipedist