Provide better explanation for updateChild and recycleChild about virtual-scroller

[valdrinkoshi]

The *Callback naming makes me think that the action already happened, and I'm notified about that, whereas using imperative naming invites the user to perform that action - that's what the scroller wants you to do if you set that property.

By default the scroller removes and destroys the children that are no longer in the view - the user can avoid destroying these by using recycleChild - the naming should suggest something on these lines. Or maybe if we do shouldDestroyChild and require to return true/false? If false, the child will be kept in the dom, and up to the user to recycle it.

[valdrinkoshi]

@graynorton do you see any drawback for 3)? I always get confused when looking at the newChild codepath :P
https://github.com/valdrinkoshi/virtual-list/blob/730c0c83b8480180f2003668ea55e5ae5a9b7ada/virtual-repeater.js#L392-L398
In general, I have a hard time justifying the need for updateChild other than "let the user know that a child was appended"

[domenic]

What about if you're not doing pooling? In that case updateChild seems pretty nice, if I understand it correctly.

[domenic]

One thing I don't quite understand: why does "recycling a child" require preventing <virtual-list> from removing it from the document?

[valdrinkoshi]

Re not doing pooling: right, that answers why we need updateChild, e.g. if we trigger a rerendering (e.g. appending an item to the array, or updating an item in the array), we don't want to trash that dom, but just update it with new data.

Recycling a child prevents virtual-list from removing the child from the document, mainly for performance reasons (we avoid a removeChild/appendChild sequence). The user can still decide to remove the child in their recycleChild function. Basically we give full control to the user on this.

[domenic]

Interesting. Yeah, in that case I think it would be great to name it "childDisconnected" (or if an event, maybe disconnectchild? childdisconnect?). You would mostly use it for child recycling, but really, it's telling you that the child is about to be disconnected, and you can prevent that if you want.

[valdrinkoshi]

re firing an event for recycleChild use case: I agree that semantically it looks more appropriate, but in terms of performance we'd be creating a new Event instance (and dispatching it) for each child we want to discard - these updates are done at each scroll event, which would mean we'll be firing multiple disconnectchild events at each scroll. I guess this is something to be measured.

In general, I see these 3 methods as one thing - something like a ChildFactory. What if we expect the user to provide a ChildFactory instance to the <virtual-list>? we would provide a base class like this:

class ChildFactory {
  newChild(item, index) {
    throw Error('newChild not provided');
  }
  updateChild(child, item, index) {
    // Override to update child.
  }
  recycleChild(child, item, index) {
    // Override to recycle child.
  }
}

The user would have to extend this base class like this:

class MyChildFactory extends ChildFactory {
  newChild(item, idx) {
    const child = pool.pop() || document.createElement('div');
    child.textContent = idx + ' - ' + item;
    return child;
  }
}
document.querySelector('virtual-list').childFactory = new MyChildFactory();

It is not mandatory for the user to extend this class, as far as childFactory has a newChild function - basically I'm suggesting to go back to what was previously called template 😁

[domenic]

these updates are done at each scroll event, which would mean we'll be firing multiple disconnectchild events at each scroll. I guess this is something to be measured.

Definitely worth measuring, yeah. I'd go with the better API design for now (events for disconnectchild/connectchild) but have an open issue to track switching back if it's a perf problem.

In general, I see these 3 methods as one thing - something like a ChildFactory.

I guess that I don't really see them this way. I see one very important method---newChild. Without this the class doesn't function at all, right?

Then I see two optional hooks that tell you about something the list is going to do anyway. The list is going to append children to the DOM, and remove them from the DOM. Maybe you want to run some code before/after it does that? OK cool, then listen for those events. That's my take, at least.

[valdrinkoshi]

I agree that we should measure the perf hit, then act on it if needed.

I see one very important method---newChild. Without this the class doesn't function at all, right?

Correct, newChild is a must have. The minimal setup requires it to be defined, and assumes items will be set only once. If items gets set multiple times, the user would have to use updateChild to update the DOM - <virtual-list> keeps the track of the created dom by mapping it to the corresponding index in the array, so it won't invoke newChild again if the new items array still has the same length, but will always invoke updateChild.
I've updated the README.md in #22 accordingly already.

[domenic]

Hmm, OK, I think I've misunderstood updateChild. Can you list all the different situations in which it is called? Here is my current guess:

• If items gets set, for all items which have corresponding DOM nodes currently connected to the DOM, updateChild(el, item, index) is called.
• If the user scrolls some items into view, causing the corresponding DOM nodes (created by newChild) to be connected to the DOM, then this is called after connecting (appending) them.

The code in virtual-repeater.js seems to kind of support this. Is this the idea?

[valdrinkoshi]

updateChild is invoked each time a reset is needed (e.g. new items array is set) or when first or num changes (e.g. user scrolls or resizes the window).

In particular, updateChild is invoked after ensuring that we have created DOM for the specified item/index and is attached. If we don't find the DOM for the given index, we create it with newChild - see the _assignChild method:
https://github.com/valdrinkoshi/virtual-list/blob/9a8ebe552ef2cc5c795b3f3b0041ba7a0857c6ac/virtual-repeater.js#L327-L332

One example where only setting newChild wouldn't work is when updating items, e.g.

list.newChild = (item, idx) => {
  const el = document.createElement('div');
  el.textContent = `${idx} - ${item.name}`;
  return el;
};
list.items = [{name: 'item'}]; // Renders correctly.

// After rendering is done...
requestAnimationFrame(() => {
  // newChild is never invoked, because
  // list._keyToChild.get(0) is defined. 
  list.items = [{name: 'updated!'}];
});

[valdrinkoshi]

We should probably use different names for these properties, what about these?

newChild --> createElement
updateChild --> updateElement
recycleChild --> recycleElement

[kenchris]

recycleElement -> disposeElement (detachElement?)? by default you are just disposing of it right, it is up to the user to implement recycling by storing the disposable element in a pool and reusing it in createElement

[domenic]

That's not my understanding; by default you aren't doing anything. It's entirely up to you.

If we wanted a name that was more agnostic toward the contents of the function, it would be something like whenElementNoLongerVisible or something, I think?

[kenchris]

I wonder if these should be called *Callback like the live cycle methods on custom elements.

Maybe it is more like leave and enter then.

I mean create will create the create the child initially (when needed the first time), and enter (updateChild) will update the data/re-render the content when entering the view (or if requestReset is called - a kind of re-render all method).

If you want to implement recycling, you detach the children in leave and re-use in create (or even in enter)

[domenic]

Callback for custom elements was a hack around the fact that we were sharing a namespace with user code. We should not repeat that pattern elsewhere on the platform.

[domenic]

By default the scroller removes and destroys the children that are no longer in the view - the user can avoid destroying these by using recycleChild - the naming should suggest something on these lines.

This is a good point. From this perspective, I think recycleChild (or recycleElement) is probably the right name. If you don't set it, you're not doing recycling. If you do, you're now responsible for it.

The alternatives that would make this even more clear are just longer variations. E.g. offscreenChildHandler or similar.

Or maybe if we do shouldDestroyChild and require to return true/false? If false, the child will be kept in the dom, and up to the user to recycle it.

I kind of liked this, but the more I thought about it, the more I realized it was just busywork for the developer, to have to remember to return things.

If we come up with some use case where it's important to know that a child has gone off-screen but you want to let it get destroyed anyway, then maybe we should use a design like this? Hmm.

[domenic]

Also, to be clear, my response to @kenchris in #25 (comment) was just factually wrong; sorry about that. With my new knowledge, disposeElement would also be reasonable. I kind of prefer recycleElement though since that nudges the user in the right direction...

Still, if we have defaults for the other methods per #80, then maybe disposeElement would fit in well...

[valdrinkoshi]

I checked what the dictionary says about "dispose" http://www.dictionary.com/browse/dispose (English is not my native language), and while I like that it could be associated to "disposable items" as in "use them before they expire", I think it might sound like we're saying to the user "please get rid of, discard this item because we can't do it".

I'd go with recycleElement as it suggests better what we need the user to do for us.