-
Notifications
You must be signed in to change notification settings - Fork 238
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
made mercurius's implicitly injected pubsub subscribe/publish methods type safe #1115
base: master
Are you sure you want to change the base?
Conversation
4470793
to
0ffc740
Compare
Signed-off-by: Matteo Collina <[email protected]>
0ffc740
to
6f18c6c
Compare
ahh i was fixing some formatting, anyway you can fix the minor details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for the types? We use tsd.
Not free right now, will try later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. I'm not a maintainer of this module though, just an active user.
export interface PubSub< | ||
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey, | ||
> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you do this, this is a breaking change, as PubSub
will require TPubSubPublishArgsByKey
to be specified:
export interface PubSub< | |
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey, | |
> { | |
export interface PubSub< | |
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey = PubSubPublishArgsByKey, | |
> { |
subscribe<TKey extends Extract<keyof TPubSubPublishArgsByKey, string>>( | ||
topics: TKey | TKey[], | ||
): Promise<Readable & AsyncIterableIterator<TPubSubPublishArgsByKey[TKey]>>; | ||
publish<TKey extends Extract<keyof TPubSubPublishArgsByKey, string>>( | ||
event: { topic: TKey; payload: TPubSubPublishArgsByKey[TKey] }, | ||
callback?: () => void, | ||
): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscribe
is a wrapper for the emitter
/ mqemitter
method on
and publish
is a wrapper for the emitter
/ mqemitter
method emit
:
export type Message = Record<string, any> & { topic: string }
on(topic: string, listener: (message: Message, done: () => void) => void, callback?: () => void): this
emit(message: Message, callback?: (error?: Error) => void): void
There is some mismatch here, both in the old and in the new types. Eg: the publish callback should be: callback?: (error?: Error) => void
I think the emitter
option of the constructor should also be specified to ensure a match between it and what's sent here:
Line 449 in a45fd77
emitter?: object; |
(Ideally the publish()
would be reimplemented as public(topic, payload, [callback])
and have some way to resolve to a promise rather than the callback, so that an await pubsub.publish('topic1', 'value1
)` would be possible, but that's out of scope of this PR)
topics: TKey | TKey[], | ||
): Promise<Readable & AsyncIterableIterator<TPubSubPublishArgsByKey[TKey]>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work properly when TKey
is something like 'key1' | 'key2'
?
export type PubSubPublishArgsByKey = { | ||
[key: string]: any; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably mimic eg MercuriusContext
and be one can extend, rather than (or in addition) to being needed to specify like PubSub<MyTopics>
– that way it will work automatically in all the places that use the PubSub
type.
The name should also be in the style of the other interfaces.
export type PubSubPublishArgsByKey = { | |
[key: string]: any; | |
}; | |
export interface PubSubTopics { | |
[topic: string]: any; | |
} |
The rationale is that mercurius implicitly injects the
pubsub
field into the context received by the graphql resolvers. For making the context type-safe we need to define a custom context type to annotate its typescript type correctly for usage in the graphql resolvers and its in that context type where we can annotate the typescript type forpubsub
object correctly for usage in the graphql resolvers by having the option of providing strictevent_name-payload
types to thepubsub.subscribe
andpubsub.publish
methods.Usage is shown in the
example.ts
file present in the root directory of this stackblitz example with proper comments.This implementation is inspired by this implementation of pubsub by graphql-yoga but its made to work with mercurius's implementation of pubsub.
Ideally mercurius should provide an explicit way of either correctly passing the types for these
event_name-payload
that are to be used inpubsub.subscribe
andpubsub.publish
functions or it should provide an explicit way of initializing the pubsub object instead of implicitly injecting it into the graphql context.