fix(core): clean up event-handling in Observable

[shirakaba]

Relates to #10534. These two PRs are independent and can be merged in either order.

This PR cleans up the event-handling in Observable. It's mostly a code-level thing, but see the details below for some minor differences in behaviour (which arguably unbreak things that had been broken to begin with).

It doesn't implement DOM Events or anything, so should be fairly uncontroversial.

The git diff is rather hard to read, so I'd recommend just reading the latest code directly.

PR Checklist

• The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/CONTRIBUTING.md#commit-messages.
• There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
• You have signed the CLA.
• All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/tools/notes/DevelopmentWorkflow.md#running-unit-tests-application.
• Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/tools/notes/WritingUnitTests.md.

What is the current behavior?

• It is possible to add duplicates of the same event (i.e. same callback, same thisArg), which is strange when compared to DOM Events.
• It is possible to add mutiple event listeners at a time (using a comma-separated list of event names) through the instance addEventListener, yet not the static one.
• There is duplicated code (once is mostly a copy-paste of addEventListener).
• There is missed opportunity for code reuse (the static and non-static implementations of removeEventListener do the same thing, yet differ).
• We trim whitespace on split-up event names in a loop rather than doing it up-front during the split operation.
• We continue to maintain these very strange static methods despite having discussed that they're probably not in use in practice.

What is the new behavior?

All of the above points are addressed.

• We now guard against adding duplicate listeners.
• Static and non-static methods now reuse code as much as possible.
• Various code cleanups.
• Deprecation notices added to the static methods.

Potentially breaking changes:

• I add an optional once?: boolean parameter to the signature of addEventListener. In theory this shouldn't be a breaking change, but in case renderers are relying on passing an arg into that (e.g. as a workaround for supporting AddEventListenerOptions), I've added a addEventListenerSchemaVersion property to Observable so that they can check at runtime whether the given version of Core follows the expected call signature.
• There is a small chance that users may have been relying on duplicate event listeners. Users can retain the original behaviour by simply passing a new callback each time they call addEventListener, rather than reusing an existing one.

[shirakaba]

I've not yet added tests, but happy to do so later (bedtime for now) – just opening it up for code review for the time being.

[CatchABus]

After a quick look, I left a comment about trim() calls. We might have to be careful about this in all event name iterations.

[shirakaba]

Looks like a couple of unit tests are failing. Will check later what’s up.

[shirakaba]

I've pushed a commit that I believe will fix the failing CI tests (I'd forgotten to pass along the once option after refactoring).

I've spoken with @farfromrefug on Discord and it sounds like neither the for of usage nor the regex-based event name trimming are blockers for him, just feedback. I like the resulting cleanliness of the code, but equally am willing to make that small change for an easy approval, so will just have a sleep on it for now.

I might as well add some unit tests characterise the things I've mentioned in the description as being fixed by these changes (which I'll need a free moment to undertake). Welcoming any more input in the meantime.

[shirakaba]

Okay, I fixed a unit test, but there's still an iOS E2E test failing (and I haven't looked at Android yet).

I figured out why the iOS test is failing. With the old behaviour, when you'd call Observable.prototype.removeEventListener or Observable.prototype.off, passing a falsy callback, it'd ignore whatever thisArg you'd pass and just remove all events matching your eventName arg.

By contrast, in Observable.removeEventlistener and Observable.off, thisArg would be used to narrow down the matches for callbacks.

The test concerns bindingContextChange, which relies on the old behaviour for instance methods. Since I brought the two in line, it caused the test to fail. I'll have to think about the best way to solve this.

[shirakaba]

Unit tests

I've added some unit tests now that set in stone some of the aspects of event listener identity that were unclear/inconsistent before.

Removing the ability to split event names

@edusperoni I agree that we should remove the functionality for splitting event names.

From my chat with @farfromrefug on Discord, we're not aware of any usage of multiple event names outside of gesture-handling in Core, so I'm optimistic that removing it would pose no risk to community plugins.

This may give us a small speed boost, as we're pointlessly splitting all event names at the moment, regardless of whether they need it.

However, I'd prefer to implement that in a follow-up PR, to keep the scope of this one small.

Trimming/splitting approach

This PR currently uses for of and regexes. This is an admitted tradeoff that prioritises readability/brevity over performance. However, I intend to remove support for adding/removing multiple event listeners at once in a follow-up PR anyway, so bikeshedding on the exact approach used here is probably unproductive.

Next steps

Awaiting review!

[shirakaba]

@CatchABus @farfromrefug @edusperoni Sorry to poke. Is anything holding this back from approval? There's much more left to clean up across Core and I'd like to move onto the next PR!

[farfromrefug]

Not on my side.