react-sortable-hoc broken in ConcurrentMode

[Jessidhia]

I haven't really made a reproduction case yet, but just enabling ConcurrentMode in my tree that includes react-sortable-hoc is sufficient to break it as it is used in my application.

Related to https://github.com/clauderic/react-sortable-hoc/issues/444.

When ConcurrentMode is enabled, the helpers that show up when dragging starts never disappear, and the SortableList also does not correctly update when a reorder does happen during a drag. After the drop, the list does reorder, but because the helper never disappears (and multiplies if you start multiple drags), they each get stuck on screen.

[clauderic]

Hey @Kovensky,

That's no good, I haven't tried using react-sortable-hoc with ConcurrentMode yet, but I will investigate.

You mentioned #444, what makes you think the issues you noticed are related to that issue?

[Jessidhia]

SortableElement does side effects in componentWillReceiveProps, which is exactly the kind of thing that is expected to break in ConcurrentMode and why it is a StrictMode warning. I believe what is happening is a race condition between SortableElement removing itself from the Manager and SortableContainer's handleSortEnd logic being invoked; as SortableElement#componentWillReceiveProps might be invoked multiple times without committing to the DOM as part of a single yieldy render.

[clauderic]

Right. I have a PR open that updates the behaviour of the disabled prop, and also changes from using componentWillReceiveProps to componentDidUpdate in SortableElement to manage the side-effects.

It's worth noting that some other changes will be required to pass strict mode.

The legacy context change is fairly minor and shouldn't be a problem, though it will require a major release as it will require bumping the minimum supported version of React:

The most concerning one is the deprecation of ReactDOM.findDOMNode, as the only replacement that wouldn't require any API changes would be Fragment refs, but I'm not sure if there's been any progress on that front. Otherwise, the API will need to be modified to allow forwarding refs.

[Jessidhia]

I haven't tested it (fixed link: https://github.com/clauderic/react-sortable-hoc/pull/472) but it does look more correct 👍

As for the remaining warnings:

• String ref can probably easily be fixed by using a function ref (or createRef, but if you do that you might as well do the legacy context migration at the same time).
• Legacy context is more of a performance / correctness issue but I don't think it'll break in ConcurrentMode, as long as context updates only happen in commit lifecycles (componentDidMount / componentDidUpdate / componentWillUnmount). AFAICT context updates don't really happen at all (createContext-wise); the manager is created once and is only internally mutated.
• findDOMNode is deprecated because it only returns the first DOM node for components that produce more than one, but it otherwise should be thread-safe to call in componentDidMount / componentDidUpdate. It's still bad to call it in other lifecycles, in render, or in event handlers.
  
  
  
  • This is also one thing that can be, for the most part, fixed through forwardRef, but that also requires the same version bump as createRef and createContext do.