chore: Replace removeEventListener with abort controller #3206
Conversation
| @@ -93,7 +93,7 @@ export function animate({ | |||
| if (event.target === element) { | |||
| // Clean up remaining inline styles | |||
| element.style.transitionProperty = ''; | |||
| element.removeEventListener('transitionstart', onTransitionEnd); | |||
| element.removeEventListener('transitionend', onTransitionEnd); | |||
There was a problem hiding this comment.
This change does not have to do anything with AbortController, I just noticed this typo
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3206 +/- ##
========================================
Coverage 96.42% 96.42%
========================================
Files 787 787
Lines 22195 22191 -4
Branches 7209 7614 +405
========================================
- Hits 21401 21398 -3
+ Misses 787 786 -1
Partials 7 7 ☔ View full report in Codecov by Sentry. |
…ts listeners are attached to specific instances
| jest.spyOn(tableRef.current!, 'removeEventListener').mockImplementation(rootRemoveEventListener as any); | ||
| abortFn(); |
There was a problem hiding this comment.
I don't see AbortController being used in table/use-table-focus-navigation.ts so the test shouldn't be changed. And it's very normal that the related test is passing because basically here you are directly calling the function and asserting that it is called below in the test. So even abort was used in the file, I don't think these two lines are really testing the same thing.
There was a problem hiding this comment.
Good catch. I initially introduced AbortController in the table/use-table-focus-navigation.ts, but then decided to get rid of it, because it didn't bring any benefit there. Addressed
Description
Replaced
removeEventListenerwithAbortController.abortwhere it brought code clarify, specifically when multiple event listeners should be removed at once.Dry-run: 7201850483
The
AbortControlleris supported injestfrom version 27. For those who use older versions (if any), should either update theirjestor use a workaround, for instance, this one: facebook/create-react-app#11626 (comment)Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.