feat(useEventListener): use `AbortController` under the hood instead of cleanup array
Before submitting the PR, please make sure you do the following
- [x] Read the Contributing Guidelines.
- [x] Read the Pull Request Guidelines.
- [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
- [x] 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
What about browser compatibility? AbortController was definitely released after ES6
What about browser compatibility?
AbortControllerwas 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
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 https://github.com/vuejs/core/pull/8763#discussion_r1260676135
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.
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.
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
@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.
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
Relevant as well is the fact that
useFetchfirst 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
AbortControllersupport
same as above
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
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.
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. 👍🏽
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
Proxyfor 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)
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?
- 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?
Should we also push it to Vue's built-in APIs like
watch?
Isn't this against the problem described by @ferferga in https://github.com/vueuse/vueuse/pull/4514#issuecomment-2603234770. with vues browser comparability? Or maybe because it is optional it should be possible?
- We clearly define the browser version range we support in the next major
not caniuse.com, but canivueuse.com 😅
@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.
This may be relevant again now that Vite and Vue browser support will be "widely available".
A PR has been opened in Vue's side: https://github.com/vuejs/core/pull/13861