Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] better type safety around networked packets #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

slice
Copy link
Contributor

@slice slice commented Jan 10, 2024

fixes PLT-1014, also closes #15.

let's mark as wip for now. not sure what the ideal scope of this PR/branch should be. definitely want to do fancy resolving of a packet's data based on the op/t you assign it so we can completely eliminate casts in app code, and collate all of our data invariants in a separate file.

The Discord API documentation describes some invariants that would be
useful to express in our types, and that are mostly true in practice -
notably, that `s` and `t` are `null` when `op` isn't `DISPATCH`.

This likely absolves us from some otherwise gratuitous null checking in
high level code. At the lower levels (i.e. when first receiving
WebSocket data), we should still perform some sanity checks to make sure
our types are OK in case of cosmic rays or freak accidents.

GatewayMessage also becomes parameterized over its data, with a default
of `unknown` (in preparation for stricter modeling). In the future,
let's automatically resolve `Data` based on `op` (if we can) to reduce
accidents.
Copy link

linear bot commented Jan 10, 2024

PLT-1014 Better type safety around networked packets

(Copied from GH)

The way we model Gateway packets right now isn't wrong:

export interface GatewayMessage {
  op: OPCode
  d?: any
  s?: number
  t?: GatewayMessageType
}

…but, we could take advantage of TS to model them in a more correct fashion. s and t are virtually guaranteed to be non-null if op == OPCode.DISPATCH, so something this could be better:

export type GatewayMessage<Data> =
  { op: OPCode.DISPATCH, data: Data, sequence: number, eventType: string }
| { op: OPCode, data?: Data }

This also:

  • renames the fields to be more human-readable
  • realizes that the data (d) of a packet should really be unknown, not any. But, since it's the primary encapsulated content of a Gateway packet, so it makes sense to parameterize over it as a generic instead

We can also make more union variants for each OPCode type, and furthermore for each eventType too, probably. This would eliminate a lot of the type annotations from business logic, make it viable to enable strict in tsconfig.json, and give us stronger typing guarantees (meaning, less bugs).

slice added 2 commits January 10, 2024 22:54
When ingesting incoming Gateway events via event emitter, have the types
correspond to the actual (believed) shape of the data. This way, the app
code can be completely free of casts, and we can encode the data shape
invariants elsewhere.

This is done by subclassing EventEmitter, and defining a new "lookup
table" type that maps each message type to the type of event data it
receives. Gateway messages that are emitted then take on the
corresponding type automatically.

Changes were made to the mapping code so that all of the types line up.
Further verification of the event data types can come in a future patch,
as I'm still not 100% confident that they align with reality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better type safety around networked packets
1 participant