use event emitters to dispatch incoming gateway events #14
Conversation
src/event-handling.ts
Outdated
| function handleReady(api: DiscordNetworkAPI, message: GatewayMessage) { | ||
| // Assert the structure of `READY`. fragile (!) | ||
| // TOOD: Move this to another file. | ||
| const d = message.d as { |
There was a problem hiding this comment.
This is unwieldy but better than refining types from any inside of function parameters IMO. This should be in another file where we also try to use the discord-api-types package as much as possible
In preparation for moving all high-level gateway event handling to a separate location, move the code handling the READY event into a separate file. Ultimately, I'm not too sure that converting class methods into exported free functions is the best idea, especially since they take an instance of the class and mutate it to boot---but the class itself was getting pretty long and it made the file a bit difficult to navigate.
slice
force-pushed
the
cleanup-event-handling
branch
from
22b2fc3 to
77d7d9f
Compare
I also just realized that this would also let us separate out guild handling instead of letting it interleave with the rest of the code, which would be really nice. This seems like a good idea, although it might involve priority-related tricks |
Naively move all code handling gateway events into subscriber functions that attach to an EventEmitter for better organization. Better types are pending.
slice
changed the title
|
OK, did a small change to expose new Gateway messages via I did improve the type annotations a little but I had to stop myself from doing a more comprehensive revamp. That can be in a separate PR (#15) |
src/network-api.ts
Outdated
| @@ -20,6 +21,7 @@ import type { DiscordEmoji, DiscordMessage, DiscordReactionDetails, DiscordScien | |||
| import _emojis from './resources/emojis.json' | |||
| import _emojiShortcuts from './resources/shortcuts.json' | |||
| import type { GatewayConnectionOptions, GatewayMessage } from './websocket/types' | |||
| import { attachReadyHandlers, attachChannelHandlers, attachGuildHandlers, attachReactionHandlers, attachMessageHandlers, attachRelationshipHandlers, attachRecipientHandlers } from './websocket/events/handlers' | |||
There was a problem hiding this comment.
this refactor is good, i like the idea of making network-api.ts not go over 500 lines.
i'm generally in favor of reducing file system nesting so wdyt of renaming events/handlers to event-handlers? i'd also suggest moving all of the handlers into one file since less friction to read all the code that way but totally up to you
There was a problem hiding this comment.
i'm generally in favor of reducing file system nesting so wdyt of renaming
events/handlerstoevent-handlers?
sgtm, i was mostly trying to account for the possible future existence of events/emitter.ts, as i was going to subclass EventEmitter for a possible .on([event₁, event₂, …]) method or .on("early" | "late", …) for priorities, etc. but i'm not totally sure if this is actually wanted/needed
i'd also suggest moving all of the handlers into one file since less friction to read all the code that way but totally up to you
factoring out the different kinds of events into separate files was one of the major goals i had in mind, as i found it p unwieldy to navigate a huge block that's responsible for handling every event possible 🤔 so i'd personally keep it like this, lmk if you'd like me to change that
There was a problem hiding this comment.
factoring out the different kinds of events into separate files was one of the major goals i had in mind, as i found it p unwieldy to navigate a huge block that's responsible for handling every event possible 🤔 so i'd personally keep it like this, lmk if you'd like me to change that
i was thinking of having all the functions in one file, that way it's not one block and easier to read. the more-files v one-large-file is another debate but since it's a cosmetic change either way is fine -- as long as you don't switch to tabs instead of spaces 😄
There was a problem hiding this comment.
ah yeah, just realized that splitting out into a bunch of different files is more friction in the text editor (more switching between tabs etc)
Less tab juggling in the text editor, while remaining easier to navigate. Less import churn too.
slice
force-pushed
the
cleanup-event-handling
branch
from
adf5243 to
3e6c951
Compare
Co-authored-by: Kishan Bagaria <[email protected]>
|
|
It's fairly unergonomic to have all (~530 LoC) of the high-level Gateway event handling code in
network-api.ts, so I think it's a good idea to move them into separate files.For now I've just moved the handling for
READYonly, but I plan to structure this more elaborately to make it easy to navigate.I'm not too sure about having them exist as free functions that receive a class instance, as that's really not too different from a method (it's literally just split off). It might be better to expose an event emitter—and then we could even separate the "update internal state" logic and "send server events up to Texts" logic (?)