Fix scroll-listener not removed on unmount

[holmberd]

https://github.com/holmberd/virtual-scroller/blob/8a5359439df3be0b71f927ced3dd0cc530b4cb0a/src/virtual-scroller.js#L264-L277

removeEventListener on LN275 is not using the bound handler reference at LN264.

Update to store a reference to the scroll event handler and use that to remove the handler on unmount.

[Arooba-git]

@holmberd Hello, just here still trying to clear my concepts of the notorious this: If I'm not wrong, the whole purpose of binding an entity in JS, is to assign a different context of this to that entity, right? But in this.#handleScroll.bind(this), this will obviously have the same value 😅..

I'm not sure how this class works, but because connectedCallback is a regular function (and not an arrow function), the context of this will change depending on how it's called.. so for e.g, if it is called directly (connectedCallback()), this in that method would refer to the entity owning it: which is the class VirtualScroller, meaning this.#handleScroll.bind(this) will interpret to VirtualScroller.#handleScroll.bind(VirtualScroller), which is redundant.

However if connectedCallback is called indirectly, via an instance (e.g objectInstance.connectedCallback()), this.#handleScroll.bind(this) will interpret to objectInstance.#handleScroll.bind(objectInstance), which, unless it is an instance of VirtualScroller itself, should cause a runtime error as objectInstance may not have a "handleScroll" method which we are trying to access.

But let's say objectInstance is always an instance of VirtualClass: in that case objectInstance.connectedCallback() would interpret this.#handleScroll.bind(this) as VirtualScroller.#handleScroll.bind(VirtualScroller) (still redundant).

I think what the developer wanted was for the this in class method handleScroll to refer to the this in connectedCallback. To achieve that, from what I know, this could be done in constructor: this.#handleScroll.bind(this.connectedCallback)

then, assuming that connectedCallback is called via a VirtualClass object, we could simply do: this.addEventListener('scroll', this.#handleScroll);

Binding in general is always done in the constructor as far as I know; as we want to avoid a function's changing contexts causing any confusion/errors.

I would appreciate any clarification/confirmation in this regard. TIA :)

[holmberd]

@Arooba-git you are correct in that the binding here is redundant, but we simply do not have to bind this at all here.

Consider the following example,

window.bar = 'window';

class Foo {
  constructor() { this.bar = 'Foo'; }

  handle() { console.log(`I am ${this.bar}`); }

  setupEventListeners() {
    // Implicitly binding this using an arrow function.
    window.addEventListener('click', (e) => foo.handle(e));
  }
}

const foo = new Foo();

window.addEventListener('click', foo.handle); // case 1
window.addEventListener('click', foo.handle.bind(foo)); // case 2
foo.setupEventListeners(); // case 3

// trigger DOM click event
// console > I am window (case 1)
// console > I am Foo (case 2)
// console > I am Foo (case 3)

In the above example for case 1 we are saying that when a click event occurs in the DOM then call the function foo.handle. The context(this) in foo.handle will be the object that fired the event, which is window, and not foo.

In our connectedCallback, which is a native browser function for when a registered custom-element, i.e. <virtual-scroller>... is connects to the DOM, we are calling this.addEventListener('scroll', this.#handleScroll.bind(this));.

When a scroll-event occurs the event-listener will call this.#handleScroll with the context being the object that fired the event, which is an instance of our VirtualScroller - which is what we want. By this logic we can remove the binding from this.#handleScroll.bind(this) and simple call this.addEventListener('scroll', this.#handleScroll) instead.

Hope that clears it up :)

[Arooba-git]

Yes, thank you for the detailed explanation! :)