[2.2.0-beta.0] Modifiers: Breaks when items are loaded asynchronously

[mike183]

When using the new modifiers syntax, it seems to break with several errors being thrown if the array of items being sorted was loaded asynchronously.

I came across this bug when implementing a component that fetches its data outside of the normal route lifecycle.

After a small amount of debugging, I found that due to the data being fetched asynchronously, this line was returning an empty NodeList.

Since the sortable-group modifier is unaware of the items being used to render the sortable-item elements, it isn't able to recognise that it needs to re-initialize itself.

Not really sure on the best approach for resolving this, any help would be appreciated 🙂

To Reproduce Steps to reproduce the behavior:

1. Create a component that fetches data asynchronously
2. Render the items using code similar to the below
3. Attempt to reorder list of items
4. Observe errors in console (errors attached at bottom of issue)

<ol {{sortable-group onChange=this.reorderItems}}>
  {{#each this.asyncFetchedItems as |item|}}
    <li {{sortable-item model=item}}>{{item.name}}</li>
  {{/each}}
</ol>

Expected behavior {{sortable-group}} modifier should be able to handle {{sortable-item}} elements being rendered asynchronously.

Additional context

Ember: v3.15.0
EmberCLI: v3.15.1
Node: v12.14.0

I've attached the errors that were thrown below,

sortable-item.js:370 Uncaught TypeError: Cannot read property 'prepare' of undefined
    at SortableItemModifier._startDrag (sortable-item.js:370)
    at SortableItemModifier._prepareDrag (sortable-item.js:335)
    at Backburner._run (backburner.js:1013)
    at Backburner._join (backburner.js:989)
    at Backburner.join (backburner.js:760)
    at join (index.js:179)
    at index.js:274

backburner.js:389 Uncaught Error: You attempted to schedule an action in a queue (actions) for a method that doesn't exist
    at DeferredActionQueues.schedule (backburner.js:389)
    at Backburner._scheduleExpiredTimers (backburner.js:1122)
    at Backburner._runExpiredTimers (backburner.js:1095)

sortable-item.js:595 Uncaught TypeError: Cannot read property 'update' of undefined
    at SortableItemModifier._drop (sortable-item.js:595)
    at sortable-item.js:362
    at Backburner._run (backburner.js:1013)
    at Backburner.run (backburner.js:754)
    at run (index.js:125)
    at drop (sortable-item.js:361)

backburner.js:389 Uncaught Error: You attempted to schedule an action in a queue (actions) for a method that doesn't exist
    at DeferredActionQueues.schedule (backburner.js:389)
    at Backburner._scheduleExpiredTimers (backburner.js:1122)
    at Backburner._runExpiredTimers (backburner.js:1095)

[lifeart]

could dom mutation observer help with it? (to handle new children) also, ie11 support could be added with https://github.com/bitovi/mutationobserver

oooor

sortable-group could have unique id and set is ad data-attr node.dataset.id = Math.random().toString(36).slice(2)

sortable-item could have shared namespace and unique id once sortable-item item appear, we can go up to node.parentNode and check for sortable-group id.

all you need - share service or state object between two modifiers and you will be able to connect them

lile:

const state = {};

// modifiers:
'sortable-group'(node) {
   const id = Math.random().toString(36).slice(2);
   node.dataset.sortableGroupId = id;
   state[id] = () => {
     /// any handler...
   }
}

'sortable-item'(node) {
   let groupId = null;
   let parent = node.parentNode;
   while(parent  && !groupId) {
      if (parent.dataset.sortableGroupId) {
        groupId = parent.dataset.sortableGroupId;
        break;
      }
      parent = parent.parentNode;
   }
   if (groupId) {
      state[groupId]('Say hello for new item!');
   }
}

[st-h]

I think I noticed a pretty related issue as well. The handle never updates its cursor, so even when the elements are not draggable, the UI still displays elements as draggable. I think we will need a way to track the handle and modify its state from the sortable-item.

Regarding the data loading issue, probably we could also track the number of the items in the group and reinitialize when it changes. However as we need a way to communicate between modifiers anyway, we should probably solve both issues the same way.

[st-h]

I gave it a stab using MutationObserver. I didn't need to define a dependency between handle and item to fixing #353 , so using a MutationObserver seemed to be the cleaner approach. Would be awesome if you could give #354 a review.

The app I am adding ember-sortable to supports async communication via web sockets, so I am pretty sure I would have run into this issue as well at some point.

[mike183]

@st-h Thanks for working on this! From a cursory glance the code seems fine, I'll try and find time to give this a test on Monday.

Only thing I can think of from the top of my head is whether or not browser support is a concern. MutationObserver is supported by IE11 (which is more than fine for me), but I'm not sure if there are any browser support targets for this add-on?

[st-h]

@mike183 maybe I am missing something, but MutationObserver seems to be supported pretty well: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver https://caniuse.com/#search=mutationobserver

But now I am wondering, which browsers work with Ember + modifiers?

On another note: I have added MutationObserver to an Addon about 2 years ago and never had any complaints regarding browser support.

[mike183]

@st-h For clarification, I'm perfectly happy with the browser support of mutation observers and using them to resolve this issue.

My comment was mainly aimed at IE, as I'm not familiar enough with this add-on to know whether or not the maintainers usually aim to support versions of IE older than 11.

[st-h]

@mike183 all good :) I meant to say that this PR only affects the modifier version of the addon and I don't think there is support for < IE11. But I wasn't able to confirm that with a quick search. If I remember correctly ember dropped support for < IE11 quite a while ago, but probably someone else has more reliable data on that.

[cah-brian-gantzler]

The way it was written is that sortable-group looks up the sortable-items. It was easier that way. I think what really needs to happen is that sortable-item registers itself with the sortable group. This would require looking up the stack for an element marked as a sortable-group. If the item is removed, remember to re-register it.

I had thought about this when writing it, if someone put an if around an item it wouldnt work.

I was making the assumption that all the items would be loaded before the sortable is rendered. My main concern is that if items are being added and removed while the user is actually dragging an item, the new sort order that is sent out on the action could be missing some times.

In your case, its async, but if the user itsnt given control to drag until all the items are loaded, anyway you could just hold off the rendering (a spinner in its place until all the items are loaded?

[st-h]

anyway you could just hold off the rendering (a spinner in its place until all the items are loaded?

That still will not work for apps that support async communication using web sockets, right?

However, I actually did not think about things like the item that is dragged could be deleted while being dragged. There still might be some weird edge cases.

[cah-brian-gantzler]

Your right. Websockets would be an issue.

When the drop occurrs, the action doesnt send what was moved where, but the entire new array in the the sorted order, which most use cases would just replace the original array and allow the for each to re-render. Any chance of other items being added or removed from the array during that could, could make the new sorted array issued by the drop action either have more items than it should or be short.

Given single threaded as long as there is no await from the time the drop to the time the reassignment of the array you might be good.

Maybe there should be an additional action that doesnt send out the reorder but instead send out the dragged item and the new desired position? It could be off by a couple if new items were added before the position, but it would be the best guess. Then it would be the users responsibility to move the item in the original array.

[cah-brian-gantzler]

I noticed that on sortable-group, register/deregister were still there even though no longer used. I removed the lookup of sortable-items on sortable-group install and instead register the sortable-items as they are created. Since the items are created before the group, had to schedule the register.

This should solve your problem in a more ergonomic way without needing to observe the mutations on the DOM

I totally stole your test btw, thanks

[st-h]

This should solve your problem in a more ergonomic way without needing to observe the mutations on the DOM

I think it there should not be much of a difference as the observer would only observe the items within the root element. (so, we usually would just be observing a bunch of li elements).

Always looking up on install has the downside that we will be doing a lot of unnecessary lookups on regular install just to fix the edge case of async loading.

Depending on the size of the list, I can image that one approach would perform better than the other - but I have not tried to run any benchmarks as the impact probably isn't that meaningful after all. So I am good with either approach.

I am bit more concerned that modifiers aren't a very good way to solve the general issue we are dealing with, but that would be more of an ember/modifier issue. Modifiers probably should be able to deal with these parent - child scenarios, but I haven't had the time so far to look into the details about modifiers.

[st-h]

I have been looking through ember-modifier a bit and it seems to me, that this would be much easier, if we kept the logic on a service (at least for the lookup stuff). ember-modifier supports DI, so there shouldn't be an issue. And we wouldn't have to deal with any unnecessary lookups or adding observers. I just asked on discord if ember-modifier should be supporting parent-child relationships, or one should use a service to deal with that. I'll update when I have some news.

[cah-brian-gantzler]

The mutationObserver when you used it took know parameters. How did it know to watch the root element and children? How did it know what the root element even was?

Technically, the component version registers itself with the parent, thats why those methods were there although it doesnt have to do the lookup, its normally only one parent away.

I specifically didnt use a service as that would be more one moving part, and possibly name conflicts. You would then have to give a name to the sortable set and assign that name to the group, item and handle or you could not support two sortables on the screen at the same time. I didnt want to have to pass that additional param. That lookup of the set on the service would probably equal the lookup of the parent at the very least.

[st-h]

The mutationObserver when you used it took know parameters. How did it know to watch the root element and children? How did it know what the root element even was?

This is the relevant code from the PR:

     this.observer = new MutationObserver(() => {
       run(() => this.initializeGroup());
     });
     this.observer.observe(this.element, {
       childList: true,
       attributes: false,
       subtree: false
     });

this.element is the root element. subtree: false specifies to not watch any subtrees

Actually there is no need to configure it that specifically as subtree: false is the default, but I thought it would make it a little easier to understand. The MDN page is quite useful here.

However, I'll be happy to close my PR, as by now I think there will be no good solution without a centralised piece that takes care of assembling the dependencies. Otherwise we will always introduce unnecessary workarounds that try to look up stuff that isn't needed. Or have to add additional watchers like the MutationObserver.

[st-h]

Is there any reason we would like to have a sortable release that just consists of modifiers?

If that is not a requirement, we could be doing things like this:

<SortableGroup @tagName='ol' as |group| >
  <li {{sortable-item @group=group}}>
    <div {{sortable-handle}}>handle</div>
  </li>
</SortableGroup>

By making the group a component we could pass the actual group to our modifiers, which would solve the initialisation issues, as we now exactly know to which group a sortable-item belongs.

But we could also try to get that information by passing the whole array to the group, when using modifiers:

<ol {{sortable-group @items=myList}}>
  {{#each myList as |myItem| }}
    <li {{sortable-item @item=myItem}}>
    </li>
  {{/each}}
</ol>

Now we would be able to know when the list (the group) changes without having to resort to additional observers. When the group modifier is installed, all item modifiers would already have been installed, so we could just iterate the list and set all items up. If a new item is added, the group notices through the @items param and we will be able to initialise any new items. Now there should be no unnecessary lookups through the dom tree, which can add up quite easily for large or multiple lists.

@cah-briangantzler what do you think about those approaches?

[cah-brian-gantzler]

The intent of the modifiers was to get rid of the component. Specifically I would like to use it with yeti-table. The group, would be the yeti-table, already a component. As a modifier I can decorate the yeti table component.

Also as a component I am forced to have another element. While ol/li might be the common use, it is not my intended use. The element would also have to be changed via tagName (not a glimmer available item). You will notice in the demo for the components, although a table is shown as an example, look close, you will notice the rows are actually indented because there is an element surrounding the tr.

I intentionally did not pass the array into the group because it really didnt need it. The group doesnt need access to the model, it needs access to the component/modifier. The only time the group needs access to the models is the drop action yields the entire array in the new sorted order. I was able to infer the list at that time. However, as I mentioned in a previous post, due to the async nature and a possible changing list, it might be better to yield the item to move and the new desired position, and allow the user to determine if the dragged item doesnt exist anymore, or if more items have been added around the desired position and can just insert there, or adjust if needed (I dont know what adjustment might need to be made but thats the point, the consumer would know not the addon). I was also concerned that yielding the entire array could be a problem if more items were added to the underlying array, just assigning the yielded array (which would not contain the new items) would lose the new items. I do not at this time intend on making that action, unless you think its a good idea, then I will.

If you really want to get rid of theDOM lookups (at the cost of another component and lookups in that component) I would suggest

<Sortable as |sortable|>
  <ol {{sortable-group @sortable=sortable}}>
    {{#each myList as |myItem| }}
      <li {{sortable-item @sortable=sortable @item=myItem}}>
         <span {{sortable-handle @sortable=sortable}} > </span>
      </li>
    {{/each}}
  </ol>
</EmberSortable>

The Sortable component would produce NO DOM, but merely yield its state. (Avoiding a service and easily allowing two different sortables on the same screen). At the cost of having to remember to pass an additional parameter into every modifier. (A {{elemenet-modifier helper like {{component would have been great and could have been yielded but that helper never came to be)

I contemplated this when originally designing the modifiers but went with the way that did not have an extra component and a param that had to passed all the time.

However, given the needs of Async, perhaps the extra param passing is the correct this to do at this time.

Does the above snippet seem acceptable to you. Also a decision on an additional onChange action

[st-h]

The element would also have to be changed via tagName (not a glimmer available item)

yeah, I was thinking we could add that in a similar way as classNames, but that indeed seems to get complicated the more I think about it.

Does the above snippet seem acceptable to you

Absolutely. I am more concerned with doing too many unnecessary things during setup than having to add another element while coding.

However, I don't think we actually need to limit this to one approach.

• If one wants best performance and doesn't mind having to pass the group around, it could look like the snippet you just posted.
• If one omits the group, then we do lookups at the time elements are installed (like your PR), which in most cases will be totally unnecessary, as modifiers down the DOM seem to be always installed first and therefore will be unable to find parent (group) modifiers (unless more items are being added after initialization)

However, we should be careful that this doesn't get too complex, when introducing different paths of execution.

[cah-brian-gantzler]

A hybrid approach wouldnt be difficult, just writing the documentation on why to use one or the other and whats the difference.

Any thought on the additional onChange action?

[st-h]

Any thought on the additional onChange action?

I am having some troubles figuring out which onChange method, as your snippet does not include one. Can you please add some more details?

[cah-brian-gantzler]

currently there is an onChange action whose params are (itemModels, draggedModel) .... hmmm I didnt realize draggedModel was there. Ok, so what would be better would be to make the params (itemModels, draggedModel, newPos) to allow the user to ignore the itemModels and do the move themselves.. I briefly looked and not sure if newPos is even accessible, but thought it might be a nice add.

LOL, now that Im typing, I guess you could always do a find on itemModels and determine the newPos yourself......

Never mind. :)

[cah-brian-gantzler]

Did we arrive at attempting to create the new component for state? then if no component is used revert back to code as it is now?

[cah-brian-gantzler]

Ok I started writing this an immediately realized, Im just moving around code. There would be pretty much zero benefits. I dont think the current implementation performs that badly.

Here is the problem. The items are inserted in the DOM before the group. I think someone alluded to that the non async items wouldnt find the group. Yet all the tests pass. Thats because if you look in the PR, the items schedule after render to find the group for this very reason.

To support a hybrid, all the logic has to remain on the group, in case the sortable component isnt present (remember this). But the fact remains the items will still be inserted before the group. If the item called a register on the sortable state, there would still have to be a schedule after render. If you call schedule several times on the same line, will it schedule them all or replace the last one? If it works, its a wash. the same code to delay the register is now on the sortable component AND the sortable-item. which means leave the schedule on sortable item.

Now you end up with

      run.schedule('afterRender', () => {
        this.args.sortableApi.registerItem(this);
      });

The registerItem on sortable looks like this

@action
registerItem(item) {
   // has to redirect to group in case this component doesnt exist.
   this.sortableGroup.registerItem(item);
}

There is also a line not shown where sortableGroup has to register itself on sortable (a simple assign) technically object.method is as much a lookup as this.element.parentElement is. But now we are looking up a method to call, which then looks up another method to call. We have added extra method calls, an entire component, and additional logic points to replace this

      run.schedule('afterRender', () => {
        let parent = this.element.parentElement;
        while (parent) {
          if (parent.sortableGroup) {
            parent.sortableGroup.registerItem(this);
            this.sortableGroup = parent.sortableGroup;
            break;
          }
          parent = parent.parentElement;
        }
      });

A lookup of this.element.parentElement and the a lookup parent.sortableGroup.registerItem (which is direct, not another indirect method call). Remember that 99% of the time, the while loop will only make one loop. I actually cant think of a time it would make more, but if someone needs more DOM between the group and the item it works.

We have two options, group looks up items (no async), items lookup group (async) but requires a schedule to give group a chance to exist. So if you want group lookup on sync, and item lookup on async I think this is the way to go to get async, and not specifying @async would default to sync

<ol {{sortable-group @sortable=sortable @async=true}}>
    {{#each myList as |myItem| }}
      <li {{sortable-item @sortable=sortable @item=myItem @async=true}}>
         <span {{sortable-handle @sortable=sortable}} > </span>
      </li>
    {{/each}}
  </ol>

This will tell group and item who is responsible for looking up who. Then you just if the lookup/register in the group and if the lookup/register in the item.

Anyone see a problem with this?

[st-h]

@cah-briangantzler ah, sorry. My bad. I totally missed the schedule after render. And to make it worse, dealing with DOM elements and native js classes all the time, I totally forgot that there still is the ember run loop that we can also use from a modifier 🤦‍♂ Now it makes sense. There shouldn't be an issue if the actual initialisation is scheduled after render, since all items should have been installed by then. Then there also is no need for an explicit async. Apologies for all the confusion.

[cah-brian-gantzler]

Does that mean you would like it left as in the PR and not give a property to control where the load is happening? Adding the extra props isnt a problem, and will give you a little finer control. Just let me know if you want me to adapt the PR or leave as is

[st-h]

@cah-briangantzler I think the option to load all items from within the group will cause unexpected errors, whenever an item is added to a list after the page has been rendered. Unless there is a performance gain or something else, I would just omit that option and load everything the way that works most reliably. It's just, that an "add item" button is sufficient to break the latest modifier release I tried, so that "async" behaviour can happen fairly easily.

[cah-brian-gantzler]

The latest release does not have PR https://github.com/adopted-ember-addons/ember-sortable/pull/355 applied yet so async does not work yet.

That PR has each item register itself instead of the group getting all the items and iterating.

We would have to wait for someone to merge that and create another beta, or you fork my repo and link temporarily to test it out if you wanted.

Your PR for the draggable fix also has not been merged https://github.com/adopted-ember-addons/ember-sortable/pull/353

[st-h]

@cah-briangantzler I am aware of that. My point was just that the "async" behaviour that has been the initial reason behind this issue, is not necessarily async and can happen fairly easily.

If the group takes care of registering, I think there always will be these "async" issues, as the group can not tell by itself that new items were added. (unless we use one of the workarounds I proposed - being unaware that we could take advantage of the runloop)

Then if the item takes only care of that async behaviour there will always be unnecessary overhead as the item doesn't know if it is initially initialising or async initialising (somewhen after everything has been initialised and rendered / when an item is added).

If now, the item takes care of registering itself to the group, by looking up after the initial render (by deferring via the runloop), it should be able to find its group on initial render and on later updates and should be fine.

My personal issue was that I tried to have the item lookup its group, but did this during install. Now the item would never find its group as the group is installed later. But, deferring this after render should work. So, I think the general approach in #355 should be fine.

Btw: you can also import from a git branch in package.json. Cutting a release is not necessary to try out a PR in your ember app. I overlooked this for way too long myself.

[alexabreu]

Running into a similar issue, and was wondering if there's been any movement on the the proposed PR.

[cah-brian-gantzler]

There is a PR https://github.com/adopted-ember-addons/ember-sortable/pull/355 out waiting to be approved. Just put a comment on it.

[mike183]

@cah-briangantzler I think you may have mistakenly posted that in the wrong repo :)

[cah-brian-gantzler]

LOL, yep. Sorry

[cah-brian-gantzler]

PR https://github.com/adopted-ember-addons/ember-sortable/pull/355 was posted to fix this issue, then there was some concern on how it looked up items for PR https://github.com/adopted-ember-addons/ember-sortable/pull/361 was posted doing the same thing but using a service. Both these PRs do the same thing just in different ways. Could we get one of them approved (or feedback) and then will close the other. Thanks

[mike183]

@ygongdev Is there anything that we can do to help get either of the above PRs merged?

[cah-brian-gantzler]

Yes would love to get this PR merged. the user in issue https://github.com/adopted-ember-addons/ember-sortable/issues/363 is extending the component to get sortable-rows and no longer would need to extend but could use the modifier on their own row component (exactly why I wrote this). Yes they are on 2.18.2 but when they get to modifiers.

[mike183]

Just another small nudge to see if there is anything I can do to help get one of these PRs merged?

I've tested #361 in my application and it seems to be working as expected. I haven't tested #355 yet, though I'm happy to do so if that would be the preferred solution.

[cah-brian-gantzler]

They both do essentially the same thing, just one does it with a service to register where the other looks it up. It would be nice to get one of the merged. We should probably just go with the service one since that what more people seemed to want it to do

[ygongdev]

Closing since PR has been merged