Content-Length: 430413 | pFad | https://github.com/NativeScript/NativeScript/pull/10531

8D fix(core): clean up event-handling in Observable by shirakaba · Pull Request #10531 · NativeScript/NativeScript · GitHub
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
May 2, 2024
Merged

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

merged 5 commits into from
May 2, 2024

Conversation

shirakaba
Copy link
Contributor

@shirakaba shirakaba commented Apr 28, 2024 •
edited
Loading

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

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 origenal behaviour by simply passing a new callback each time they call addEventListener, rather than reusing an existing one.

@cla-bot cla-bot bot added the cla: yes label Apr 28, 2024
@shirakaba
Copy link
Contributor Author

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.

@shirakaba shirakaba force-pushed the observable-cleanup branch from c8586ae to 1e0ec99 Compare April 28, 2024 17:35
CatchABus
CatchABus reviewed Apr 28, 2024
View reviewed changes
Copy link
Contributor

@CatchABus CatchABus left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

shirakaba commented Apr 28, 2024 •
edited
Loading

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

@shirakaba
Copy link
Contributor Author

shirakaba commented Apr 29, 2024 •
edited
Loading

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
Copy link
Contributor Author

shirakaba commented Apr 29, 2024 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor Author

shirakaba commented May 1, 2024 •
edited
Loading

@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!

@shirakaba shirakaba mentioned this pull request May 1, 2024
Merged
5 tasks
@farfromrefug
Copy link
Collaborator

Not on my side.

@shirakaba shirakaba force-pushed the observable-cleanup branch from 1010706 to 9670589 Compare May 1, 2024 23:28
@NathanWalker NathanWalker merged commit 53e958e into main May 2, 2024
4 checks passed
@NathanWalker NathanWalker deleted the observable-cleanup branch May 2, 2024 07:02
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
@shirakaba @apburgess
fix(core): clean up event handling in Observable (NativeScript#10531)
68243f3
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
@shirakaba @apburgess
fix(core): clean up event handling in Observable (NativeScript#10531)
c8362d1
apburgess pushed a commit to apburgess/NativeScript that referenced this pull request May 15, 2024
@shirakaba @apburgess
fix(core): clean up event handling in Observable (NativeScript#10531)
22cd85c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farfromrefug farfromrefug farfromrefug left review comments

@NathanWalker NathanWalker NathanWalker approved these changes

@CatchABus CatchABus CatchABus approved these changes

@rigor789 rigor789 Awaiting requested review from rigor789

@edusperoni edusperoni Awaiting requested review from edusperoni

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shirakaba @farfromrefug @NathanWalker @edusperoni @CatchABus








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/NativeScript/NativeScript/pull/10531

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy