Use latest on/off callbacks from a particular caller

[greghuc]

This PR addresses an edge case of on-load that has unintuitive behaviour. Specifically:

• onload is called multiple times on an element, by the same caller
• each onload call is made with new on and off functions (compared to the previous call)
• when the off callback is eventually triggered on the element, the oldest off function supplied is called. This is unintuitive, as the off function was superceded in later calls to onload.

This PR changes the behaviour to use the latest off function supplied instead, which seems more intuitive. It would also use the latest on function too, but I don't think this callback would ever called.

The PR changes

function eachAttr (mutation, on, off) {
  ..
  if (sameOrigin(mutation.oldValue, newValue)) {
    watch[newValue] = watch[mutation.oldValue]
    ..
  }

to

function eachAttr (mutation, on, off) {
  ..
  if (sameOrigin(mutation.oldValue, newValue)) {
    watch[newValue][2] = watch[mutation.oldValue][2]
    ..
  }

So previously the latest watch array for an element was overwritten with the earlier watch array; this means that the earlier on/off functions are always used in element callbacks. In this PR, we only overwrite watch[newValue][2] with watch[mutation.oldValue][2], which I believe indicates whether on or off was called last. So the later on/off functions are used as element callbacks instead.

[greghuc]

Also thinking about this, is there any reason we wouldn't delete watch[mutation.oldValue] in this case? Otherwise, there'll be a memory leak overtime as a caller makes multiple onload calls for the same element.

[bcomnes]

@shama or @lukeburns do you have any thoughts on this?

[bcomnes]

@greghuc seems like this is a good improvement, and covers some subtle characteristics about how on-load works for the better. Do you see any drawbacks from this? Nothing is coming to mind when looking at this.

[bcomnes]

We can cut this in as a patch or minor in 4.0.0 after we discuss further.

[greghuc]

@bcomnes I made this PR ages ago, and can't remember all the details. However, my former self did put serious work into understanding how on-load worked, so I do trust this PR and the suggestion to delete watch[mutation.oldValue]. I still use on-load, so it's great it's getting some love.

[lukeburns]

@bcomnes this looks good to me! 👍

[greghuc]

Hi @bcomnes.

I'm not sure if you're still maintaining the on-load package, but if you are.. could you consider merging this (just updated) pr from 2016?

I just touched some code that uses on-load, so I decided to update this pr. The pr has tests and was ok-ed by @lukeburns in 2018. I also updated everything to 2023 by a) updating deps and b) linting with standard.

The underlying pr was two changes:

• One liner index.js change
• Single test case

Many thanks!

Greg

[shama]

Thanks @greghuc. Tested locally and LGTM. I'll merge this and do some other clean up then publish as new major (because of the var to let changes. Even though 6+ years later now let is well supported now, still technically a breaking change).

[greghuc]

@shama many thanks!