refactor(events): introduce EventType enum in separate module
#8530
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce an enum for the event .type values. We can't actually
use it as the type of the .type property on Abstract events,
because we want to allow developers to add their own custom
event types inheriting from this type, but at least this way we
can be reasonably sure that all of our own event subclasses have
distinct .type values—plus consistent use of enum syntax
(EventType.TYPE_NAME) is probably good for readability overall.
Put it in a separate module from the rest of events/utils.ts
because it would be helpful if event utils could use
event instanceof SomeEventType
for type narrowing but but at the moment most events are in
modules that depend on events/utils.ts for their .type
constant, and although circular ESM dependencies should work
in principle there are various restrictions and this
particular circularity causes issues at the moment.
A few of the event classes also depend on utils.ts for fire()
or other functions, which will be harder to deal with, but at
least this commit is win in terms of reducing the complexity
of our dependencies, making most of the Abstract event subclass
module dependent on type.ts, which has no imports, rather than
on utils.ts which has multiple imports.
gonfunko
approved these changes
Aug 19, 2024
1 task
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basics
The details
Proposed Changes
Introduce an
enumfor the event.typevalues.Reason for Changes
Although we can't actually use it as the type of the
.typeproperty on theAbstractevent class, because we want to allow developers to add their own custom event types inheriting from this type, this change means we can be reasonably sure that all of our own event subclasses have distinct.typevalues—plus consistent use of enum syntax (EventType.TYPE_NAME) is probably good for readability overall.Additionally, by putting it in a separate module from the rest of
events/utils.ts, it moves us a step towards being able to write type narrowing checks likein
utils.ts, which is not currently possible due to load ordering problems that arise when there are (non-type) circularimports betweenutils.tsand the other modules inevents/.(A few of the event classes also depend on
utils.tsforfire()or other helper functions; this will be harder to untangle, but at least this commit is a step forward in terms of reducing the circularity of of our dependencies, making most of the event subclass modules dependent ontype.ts, which has no imports, rather than onutils.tswhich has multiple imports.)Test Coverage
Passes
npm test; no changes to manual testing expected.Documentation
No; API-exported symbols remain unchanged.
Additional Information
I've also restructured
events/events.tsto useexpor … from …for readability and conciseness.