perf(useEventListener): use `AbortController` under the hood instead of cleanup array #4514

OrbisK · 2025-01-20T09:12:39Z

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

⚠️ Slowing down new functions

Warning: Slowing down new functions

As the VueUse audience continues to grow, we have been inundated with an overwhelming number of feature requests and pull requests. As a result, maintaining the project has become increasingly challenging and has stretched our capacity to its limits. As such, in the near future, we may need to slow down our acceptance of new features and prioritize the stability and quality of existing functions. Please note that new features for VueUse may not be accepted at this time. If you have any new ideas, we suggest that you first incorporate them into your own codebase, iterate on them to suit your needs, and assess their generalizability. If you strongly believe that your ideas are beneficial to the community, you may submit a pull request along with your use cases, and we would be happy to review and discuss them. Thank you for your understanding.

Description

This PR removes the cleanup array and replaces it with an abort controller.

Ref: https://kettanaito.com/blog/dont-sleep-on-abort-controller

Additional context

…of cleanup array

ferferga · 2025-01-20T19:14:15Z

What about browser compatibility? AbortController was definitely released after ES6

OrbisK · 2025-01-20T20:57:53Z

What about browser compatibility? AbortController was definitely released after ES6

Correct me if I am wrong, but AbortController should not be related to es6? Or are you talking about the timeline? 😅

Source: https://developer.mozilla.org/en-US/docs/Web/API/AbortController/AbortController

ferferga · 2025-01-20T21:11:54Z

I mean that I think we should have the same browser support as Vue 3, which is exactly ES6. Merging this means that we request more recent browser support than Vue 3 itself.

See also vuejs/core#8763 (comment)

The question here it's what's the browser support baseline we want for VueUse? I don't see that mentioned everywhere, but as a consumer of this library, I expect it should have the same support as Vue 3 itself in the core composables and return isSupported in the composables that use modern APIs.

In my opinion, here it's not necessary to return isSupported, but we should handle a fallback for old browsers. So it's a matter of internally calling useSupported checking the existence of AbortController and testing if the signal option is also valid (which was introduced later in Chrome 90, as seen in MDN

In my opinion, not sure how worth all of this is though for the increased complexity.

OrbisK · 2025-01-21T09:25:21Z

Okay, got it.

In my opinion, not sure how worth all of this is though for the increased complexity.

The question is how to detect if addEventListener supports options.signal

Perhaps we need to check existing composables. useFetch (+useAsyncQueue) also use the AbortController signal, which is also chrome>=66.

43081j · 2025-01-21T11:30:46Z

packages/core/useEventListener/index.test.ts

@@ -136,16 +132,6 @@ describe('useEventListener', () => {
        listeners.forEach(listener => expect(listener).toBeCalledTimes(index + 1))
      })
    })
-
-    it('should remove all listeners with all events', () => {


maybe you could have an equivalent test for an abort signal, and just assert signal.aborted is true?

i know we already test teardown by asserting calls are 0, but its probably still worth testing the signal got aborted

Not that easy. We have to mock AbortSignal to do this, no?

43081j · 2025-01-21T11:37:45Z

i've approved but i agree we should decide what our level of support is

if we don't care about engines old enough that they don't have AbortController, this looks good and we should do it in more places

if we do care, i'd consider holding off for now rather than having a mixture of cleanup/abort in the repo

looks like useFetch is the only one using it

ferferga · 2025-01-21T11:56:35Z

@OrbisK You need to surround the addEventListener(..., { signal }) in a try/catch, if it catches, then it's unsupported.
@43081j Relevant as well is the fact that useFetch first checks for support, so definitely something we want to do here.

43081j · 2025-01-21T12:02:01Z

keep in mind it seems like useFetch uses it to setup its own abort function the user can call

so when AbortController doesn't exist, it simply makes abort a noop function rather than falling back to anything

we have a slightly different use case here, since we're using it to clean up rather than abort something. we'd end up with a memory leak in browsers which don't support it if we did that.

that means in our case, we'd need a fallback (removeEventListener). which makes this change redundant/bloated. i think we can only land this if we agree we require AbortController support

OrbisK · 2025-01-21T12:13:23Z

Relevant as well is the fact that useFetch first checks for support, so definitely something we want to do here.

useFetch checks for AbortController (chrome>=66), but we need to check for https://caniuse.com/mdn-api_eventtarget_addeventlistener_options_parameter_options_signal_parameter (chrome>=90)

i think we can only land this if we agree we require AbortController support

same as above

ferferga · 2025-01-21T12:20:21Z

but we need to check for https://caniuse.com/mdn-api_eventtarget_addeventlistener_options_parameter_options_signal_parameter (chrome>=90)

@OrbisK see my comment above

OrbisK · 2025-01-21T12:44:44Z

but we need to check for https://caniuse.com/mdn-api_eventtarget_addeventlistener_options_parameter_options_signal_parameter (chrome>=90)

I rather mean that with useFetch it is already known initially, so to speak, whether it is supported. In this case, we would only know if the addEventListener fails.

OrbisK · 2025-01-21T20:13:28Z

I thought about this a bit more. useEventListener is one of the composables that is used most often in other composables. So the impact would be very high, both positive and negative.

Of course, the different browser compatibility with vue speaks against this. On the other hand, according to caniuse, options.signal is supported in 94.99% of browsers and is considered a standard.

With options.signal, it would also be possible to replace the cleanup arrays in dependent composables with a simple signal.

I don't know what's right here.

Maybe tomorrow I will try to see how a fallback would feel and work. 👍🏽

ferferga · 2025-01-22T00:39:09Z

Some key points:

Performance: We assumed beforehand it's better because it's less loops, but it's that the case? Perhaps the browser internally does the same thing? (Spoiler, it is faster¹, but not crazy fast anyway, JSBenchmark)
Does the benefit outweights by far the tradeoffs?: Your numbers are for evergreen browsers only, which are indeed correct. However, lots of business or applications must target older devices. See Vue's RFCs about dropping Internet Explorer. You can see in the comments how there was some backlash. In that case, the benefits outweighted the tradeoffs (using Proxy for reactivity made it possible to work with objects as if they were native and have reactivity working where we didn't expect it before!). Internet Explorer is an extreme case, but many business have the requirement to have as broader compatibility as possible. I've seen some threads in the internet about people using Vue for medical/ATM frontends (which are not running evergreen browsers) and, the nearest case it's me: in Jellyfin we need to support ancient Chrome versions, since TVs usually come with old Chromium/Webkit engines (I'm specifically talking about jellyfin-web, jellyfin-vue doesn't care about backwards compatibility and only targets evergreen browsers with ES2024 support as of the time of writing. However, one of the reasons that completely plummetted any chance of replacing jellyfin-web with jellyfin-vue was the drop of pre-ES6 compat). So, in some cases like jellyfin-vue, backwards compat doesn't matter (and it's even counterintuitive, since backwards-compat usually means bigger bundles), because the target users are expected to be evergreen browsers only*, but a lot of people has applications that need to run both in evergreen browsers and in embedded/managed devices.

To sum up my points:

We should set Vue 3's as our bar for browser compatibility while providing fallbacks for everything else (I believe we're all in this page)
Our expectations for backwards-compatibility shouldn't rely on stats sourced by user traffic or evergreen browsers alone, unless we rely in stats that include disconnected devices, IoT and such.
If you came up with a solution that checks properly all needed support, it's still faster than the cleanup array including the checks (you can modify my benchmark for testing) and keeps around the same LoC (it's still easy to maintain + keeps bundle light), I fully agree with this. Otherwise, as stated above, I don't think the benefits will outweight the tradeoffs since the performance improvements are not so noticeable in smaller sets, only in bigger (probably unrealistic) amounts. ¹

¹ In fact, it's even slower with a single event, JSBenchmark), 5 events JSBenchmark, I've only see it better for 10+ events, which are really low for a single composable usage (a global AbortController that handles all would be great, but not possible in this case)

antfu · 2025-01-22T07:51:03Z

I am also concerned with the browser support. This also relies on whether the targets' addEventListener supports signal or not.

I agree that in the long term this seems to be a more elegant solution to clean up events. To move forward, I think:

We should hold this PR to the next major
We clearly define the browser version range we support in the next major
With the next major, we should adapt to it everywhere for consistent

A bit out of topic: I also wonder if we should build a practice around AbortController, like accepting a controller for most of the composable to replace the stop handler? Should we also push it to Vue's built-in APIs like watch?

OrbisK · 2025-01-22T11:07:01Z

We clearly define the browser version range we support in the next major

When we do, I was looking for a polyfill to reference in docs like: https://github.com/nuxodin/lazyfill/blob/main/monkeyPatches/addEventListenerSignal.js

Maybe we can still get the fallback variant to work if we somehow discover support for all use cases once, like this:

https://github.com/nuxodin/lazyfill/blob/c53e43fe2d88269cf84b924461218c23422cc49a/monkeyPatches/addEventListenerSignal.js#L5-L8

With the next major, we should adapt to it everywhere for consistent

Should I do this within this PR? or create branches from this one?

OrbisK · 2025-01-22T11:23:21Z

Should we also push it to Vue's built-in APIs like watch?

Isn't this against the problem described by @ferferga in #4514 (comment). with vues browser comparability? Or maybe because it is optional it should be possible?

OrbisK · 2025-01-23T07:27:38Z

We clearly define the browser version range we support in the next major

not caniuse.com, but canivueuse.com 😅

ferferga · 2025-01-23T07:32:03Z

@antfu So basically an stoppable effectScope using AbortController as well? Sounds cool!

@OrbisK Given it will be an optional parameter, and logic for detecting if AbortController is supported or not in effectScope can be added, I think it could be done.

perf(useEventListener): use AbortController under the hood instead …

dd92d3f

…of cleanup array

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 20, 2025

OrbisK marked this pull request as draft January 20, 2025 10:05

OrbisK added 2 commits January 20, 2025 10:57

fix: renew AbortController if options changed

18705a3

test: update dependant tests

143fa59

OrbisK marked this pull request as ready for review January 20, 2025 12:53

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 20, 2025

43081j reviewed Jan 21, 2025

View reviewed changes

43081j approved these changes Jan 21, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 21, 2025

antfu added this to the 13.0 milestone Jan 22, 2025

antfu added the BREAKING CHANGE label Jan 22, 2025

OrbisK mentioned this pull request Feb 10, 2025

feat: add useWebLocks (Web Locks API) and useLeaderElection #4574

Draft

4 tasks

OrbisK removed this from the 13.0 milestone Feb 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

perf(useEventListener): use `AbortController` under the hood instead of cleanup array #4514

perf(useEventListener): use `AbortController` under the hood instead of cleanup array #4514

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perf(useEventListener): use AbortController under the hood instead of cleanup array #4514

Are you sure you want to change the base?

perf(useEventListener): use AbortController under the hood instead of cleanup array #4514

Uh oh!

Conversation

Uh oh!

Before submitting the PR, please make sure you do the following

Description

Additional context

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perf(useEventListener): use `AbortController` under the hood instead of cleanup array #4514

perf(useEventListener): use `AbortController` under the hood instead of cleanup array #4514