[base-controller] `ControllerMessenger` should prohibit retroactive subscriptions to published events
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 413
- Forks
- 308
- Avg merge
- 1d 4h
- Merged PRs (30d)
- 253
Description
## Motivation
Listeners in typical event systems like `EventEmitter` only trigger if they existed when the event was emitted. However, the messenger event pubsub system in our `ControllerMessenger` class does not follow this norm. This behavior should be fixed for maintenance and predictability reasons.
## Acceptance Criteria
- Subscribers should receive events only if they were already registered to the event at the time it was published.
- After a given event has been published, its subscribers collection should not be retroactively mutated.
- Eliminate the risk of a mutable subscribers collection triggering an infinite loop in React/Redux due to a circular dependency (e.g. a subscription handler mutates a hook dependency, which re-triggers its subscription).
## Further Research
- Consider introducing "Topics" for a collection of events that may need to be published repeatedly, potentially to a growing list of subscribers.
## References
- Proposed by @matthewwalsh0:
> Currently, when an event is published, we iterate through all the current `subscribers` using `entries()`.
This returns an iterator, meaning that the subscribers collection is dynamically processed, and so if any of the listeners add an additional subscriber, it too will be processed in the same loop, even though the event was technically published before the new listener existed.
I worry this is undesirable conceptually and technically, as in typical event systems (like the legacy `EventEmitter` we used), listeners are only triggered if they existed when the event was emitted.
This issue has already presented itself within a recent mobile bug I was looking at where we were did a `subscribe` within a `useEffect` as the listener relied on dynamic React properties and Redux state. As the listener ultimately mutated the dependencies that re-triggered the subscription, the above behaviour meant that the subsequent listeners were executed immediately and crashed the app due to a render loop.
Whether the above example is good practice is a separate issue as I've resolved it by refactoring the dependencies, but I still feel this publish behaviour isn't ideal simply for the maintenance and predictability implications.
I've confirmed this was the issue by updating my `node_modules` directly to process the subscribers upfront with a `[...entries()]`.
> -- https://consensys.slack.com/archives/C01V1L10W2E/p1726146922994539
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at the ControllerMessenger publish and subscription behavior described in the issue, then inspect the surrounding implementation and existing tests. Reproduce a subscription added during publication and verify that it is not invoked for the current event; done means existing subscribers run once and the subscriber collection is not retroactively processed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100