feat: add extra caller property to onSelectedRowsChanged

[ghiscoding]

• this is to cover a special use case that we have, in which the user can click a row which will possibly trigger other rows to be selected (in a Tree Data grid, we want to select all the children underneath), so we basically dynamically change row selection via setSelectedRows and the current code was problematic because internally SlickRowSelectionModel is also calling setSelectedRows and we have no way of knowing if the selection was made by the user or dynamically by the code, so this PR adds a caller property which tells us who really called the row selection.
• while at it, I also added extra properties, 3x in total: (caller, changedSelectedRows and changedUnselectedRows), this makes my code a lot cleaner since I often have to add extra code to know the last changes

Now the onSelectedRowsChanged provide extra properties, the caller property is really helpful in my use case

[ghiscoding]

@6pac could you review and provide feedback to this PR please.
I had to resolve to cheating code to get this working, I'm passing extra data via a SlickEventData (which was not made for that), but I don't see any other ways to pass extra data since onSelectedRowsChanged and onSelectedRangesChanged only resolve arrays of SlickRange and/or row number. We typically return an object that we can extend in the future but these events only have arrays which we can't really extend and so no ways of providing extra stuff which is why I resolved to using cheat code. Are you ok with the change? Do you have any other suggestions that might work?

[6pac]

From what I can see, the calling pattern for onSelectedRowsChanged in the core grid is:

  if (simpleArrayEquals(previousSelectedRows, selectedRows)) {
    trigger(
        self.onSelectedRowsChanged, 
        {   rows: getSelectedRows(), 
            previousSelectedRows: previousSelectedRows
        }, 
        e);
  }

So this uses the 'args-is-an-object' convention, and you should be able to just add the property.
For onSelectedRangesChanged, it appears only to be used in cellselectionmodel and rowselectionmodel (though it may also be called by user code), with the call:

  _self.onSelectedRangesChanged.notify(_ranges);

This doesn't follow the convention of 'args-is-an-object', which really should be corrected, but that would be a breaking change. The only non-breaking way I can think of is to take advantage of the fact that in js an array is also an object. So we could do, for example:

  var args = _ranges;
  args.ranges = _ranges;
  args.caller = caller;
  _self.onSelectedRangesChanged.notify(args);

a bit of a hack, but technically correct. It may be confusing when looking in the debugger, but I think a comment at the call would be enough to demystify that.

[ghiscoding]

hmm I can give a try next week but I'm not crazy about this approach and I don't think that would work because the other side is really expecting an array not an object, but since nearly everything is an object in JS we never know, I'll give a try next week. If that doesn't work, my approach, even if not ideal, is still working and for the most part this code is used only internally (to communicate between SlickGrid & the plugins) so we might be ok since it's not that expose to the outside user.

[6pac]

Sure. What I'm suggesting is that in internal code we treat it as an object. Setting it as an array is only to maintain backwards compatibilty for legacy user calls.

[6pac]

Given that my proposed solution is a hack, I think yours works equally well, so either-or. The issue with onSelectedRangesChanged does need to go on the list for when we finally release a breaking version.

[ghiscoding]

@6pac yeah I would rather merge it as it is, I know it works with use case in our Salesforce environment and so I would rather use that for now. It's mostly an internal thing but you never know when someone hooks into any of the onSelectedRangesChanged events (I'm sure there's someone in the world doing that) so I would rather not break them and my solution even though hacky as well isn't too bad and it works without breaking anything

[6pac]

OK, so we are ready to merge this and release?

[ghiscoding]

wait give me another 5min, I'm giving a try with your hack which is basically this SO

[ghiscoding]

it seems to work too, but I'm not sure how that would work with TypeScript. I would say let's merge with the code I have now (it works with TypeScript in my lib) and we can keep it as a 2nd option. Let me merge my PR then

[6pac]

Yeah, I don't think Typescript would like it. As they say in the SO post, it's not that it won't work, it's just quite an unusual pattern and might be quite confusing for some developers. OK, willl pull and release.

[ghiscoding]

@6pac I merged the PR, you can go ahead with a release at your convenience