#7675: Reuse SimpleEventTarget wherever possible
#7765
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7765 +/- ##
==========================================
- Coverage 72.22% 72.20% -0.02%
==========================================
Files 1270 1270
Lines 39735 39681 -54
Branches 7369 7365 -4
==========================================
- Hits 28697 28652 -45
+ Misses 11038 11029 -9 ☔ View full report in Codecov by Sentry. |
| */ | ||
| // eslint-disable-next-line local-rules/persistBackgroundData -- Functions | ||
| const _navigationListeners = new Set<() => void>(); | ||
| const navigationListeners = new RepeatableAbortController(); |
There was a problem hiding this comment.
Also here RepeatableAbortController was useful rather than SimpleEventTarget
src/registry/memoryRegistry.ts
Outdated
| listener.onChanged(); | ||
| } | ||
| } | ||
| const databaseChangeListeners = new SimpleEventTarget(); |
There was a problem hiding this comment.
This file has two events and two notifyListeners loops. replaced with:
databaseChangeListenersMemoryRegistry#onCacheChanged
Feel free to suggest alternative names.
There was a problem hiding this comment.
These events correspond to:
- databaseChangeListeners -> packageRegistryChange (the background package registry in IDB has changed)
- MemoryRegistry -> change (the memory registry's content may have changed, e.g., because the background package registry in IDB has changed)
I will be cleaning up some of the registry architecture next as part of the platform API work. The MemoryRegistry class is currently tightly coupled to the background API and also the assumption it can enumerate all packages
The likely changes will be:
- Having RegistryProtocol and EnumerableRegistryProtocol
- Splitting out MemoryRegistry from its backing data source (either IDB, or lazily fetching)
There was a problem hiding this comment.
- databaseChangeListeners -> packageRegistryChange
Renamed ✅
- MemoryRegistry -> change
This is used as brickRegistry.onCacheChanged.add(callback) right now so it might not be super readable as brickRegistry.change.add(). I'm open to changing it anyway.
There was a problem hiding this comment.
Perhaps onChange or changeEvent to make it clear it's an event from the name?
| @@ -206,7 +206,7 @@ export abstract class MenuItemStarterBrickABC extends StarterBrickABC<MenuItemSt | |||
| * @see MenuItemStarterBrickConfig.dependencies | |||
| * @private | |||
| */ | |||
| private readonly cancelDependencyObservers: Map<UUID, () => void>; | |||
| private readonly cancelDependencyObservers = new EventTarget(); | |||
There was a problem hiding this comment.
Here I used a regular EventTarget because:
- one observer must be fired only for a specific
extension.id - the listener is removed via
once: true- it looked like every call was "call and remove from list", so
onceshould match that
- it looked like every call was "call and remove from list", so
There was a problem hiding this comment.
Can you leave a comment in the code why you're using EventTarget and not the usual SimpleEventTarget?
| // Highlight the exact item being hovered, but | ||
| // don't highlight it when its children are hovered | ||
| li:hover:not(:has(li:hover)) { | ||
| background-color: $color-hover; |
There was a problem hiding this comment.
Tested. Drops all the mouseenter/mouseover listeners from useTreeRow.ts
There was a problem hiding this comment.
By the way, the up/down keys don't work :(
| isHMR && | ||
| new ReactRefreshWebpackPlugin({ | ||
| overlay: false, | ||
| }), |
There was a problem hiding this comment.
Unrelated, I'll be tweaking HMR over time. I'm seeing some serious issues (reload loops on most websites)
What does this PR do?
This removes the last custom "event listener" implementations
Checklist