SortableJS / react-sortablejs

React bindings for SortableJS
http://sortablejs.github.io/react-sortablejs/
MIT License
2.01k stars 208 forks source link

[bug] Basic demo not working when simulating a mobile device (either via the Chrome console or an iOS simulator) #241

Open scouttyg opened 2 years ago

scouttyg commented 2 years ago

Describe the bug In a nutshell, I recreated a basic demo of react-sortablejs within next.js (see here: https://github.com/scouttyg/sortablejs-nextjs-test-bug), but for some reason things don't seem to work when I go into Chrome Console and click the "Toggle device toolbar" setting.

When I'm just in normal desktop mode, things work perfectly! You can see the demo working in the gif below: sortablejs-desktop

The minute I enable the "Toggle device toolbar" setting however (e.g. on Chrome), or when I try to sort in Mobile Safari on the iOS simulator, things don't seem to drag / drop: sortablejs-devicetoolbar

To Reproduce Steps to reproduce the behavior:

I've created a small repo to reproduce the issue: https://github.com/scouttyg/sortablejs-nextjs-test-bug

If you clone that repo, run yarn install, then yarn dev, go to localhost:3000, and then (on Chrome) enable the developer console and click "toggle device toolbar", the items won't be able to be dragged / dropped.

Expected behavior I'd expect things to sort regardless of whether it was simulating a mobile environment or not.

Information

    "react-sortablejs": "^6.1.4",
    "sortablejs": "^1.15.0"

Additional context Interestingly enough, when I build my app to production (e.g. see here: https://sortablejs-nextjs-test-bug.vercel.app/) it works perfectly on both desktop and mobile devices, so I'm confused if I'm just doing something wrong, or something in development is interfering, or if it's an issue with next.js, or some other thing.

scouttyg commented 2 years ago

After debugging more, I seemed to have narrowed it down to the following:

I can also get it to work using "react": "18.0.0" or above if I set:

const nextConfig = {
  reactStrictMode: false,
}

So my guess is something is not behaving correctly in strict mode within the library?

ianldgs commented 2 years ago

I definitely have issues with this library in React 18 too. Even on desktop.

AlexSterk commented 10 months ago

Ran into this problem today. I know it's old but I decided to dig a little deeper.

It's because the component is rendered twice in strict mode, causing

  componentDidMount(): void {
    if (this.ref.current === null) return;
    const newOptions = this.makeOptions();
    Sortable.create(this.ref.current, newOptions); // running this twice
  }

to run twice.

This registers the event handlers to the same target twice, which causes the following code from Sortable to return early when an event is fired

https://github.com/SortableJS/Sortable/blob/master/src/Sortable.js#L473-L476

Not sure why/if it's different between desktop and mobile, but I think this is the underlying cause.

you can recreate this behavior in a plain website with just SortableJS:

const list = document.getElementById('list');
Sortable.create(list, {});
Sortable.create(list, {});

this causes the same bug.

I think the fix would be to destroy the Sortable when unmounting the component, but I'm not familiar enough with class components/strict mode/SortableJS to say that for sure. But I guess since strict mode is disabled for production anyway, there is no urgency to fix this.