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.

Sign up for GitHub

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

Jump to bottom

made mercurius's implicitly injected pubsub subscribe/publish methods type safe #1115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

made mercurius's implicitly injected pubsub subscribe/publish methods type safe #1115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,20 @@
type Mercurius = typeof mercurius

declare namespace mercurius {
export interface PubSub {
subscribe<TResult = any>(topics: string | string[]): Promise<Readable & AsyncIterableIterator<TResult>>;
publish<TResult = any>(event: { topic: string; payload: TResult }, callback?: () => void): void;
export type PubSubPublishArgsByKey = {
[key: string]: any;
};

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Extra semicolon

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Extra semicolon

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Extra semicolon

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Extra semicolon

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Extra semicolon

Check failure on line 28 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Extra semicolon
Comment on lines +26 to +28
Copy link
Contributor

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.

Suggested change
export type PubSubPublishArgsByKey = {
[key: string]: any;
};
export interface PubSubTopics {
[topic: string]: any;
}


export interface PubSub<

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Unexpected trailing comma

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Unexpected trailing comma

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Unexpected trailing comma

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Unexpected trailing comma

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Unexpected trailing comma

Check warning on line 30 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Unexpected trailing comma
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey,
> {
Comment on lines +30 to +32
Copy link
Contributor

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:

Suggested change
export interface PubSub<
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey,
> {
export interface PubSub<
TPubSubPublishArgsByKey extends PubSubPublishArgsByKey = PubSubPublishArgsByKey,
> {

subscribe<TKey extends Extract<keyof TPubSubPublishArgsByKey, string>>(

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 33 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 4 spaces but found 6
topics: TKey | TKey[],

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 34 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 6 spaces but found 8
): Promise<Readable & AsyncIterableIterator<TPubSubPublishArgsByKey[TKey]>>;

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 35 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 4 spaces but found 6
Comment on lines +34 to +35
Copy link
Contributor

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'?

publish<TKey extends Extract<keyof TPubSubPublishArgsByKey, string>>(

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 36 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 4 spaces but found 6
event: { topic: TKey; payload: TPubSubPublishArgsByKey[TKey] },

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 37 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 6 spaces but found 8
callback?: () => void,

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 6 spaces but found 8

Check failure on line 38 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 6 spaces but found 8
): void;

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, macOS-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (22.x, windows-latest) 

Expected indentation of 4 spaces but found 6

Check failure on line 39 in index.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x, windows-latest) 

Expected indentation of 4 spaces but found 6
Comment on lines +33 to +39
Copy link
Contributor

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:

mercurius/index.d.ts

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)

}

export interface MercuriusContext {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "mercurius",
"version": "14.1.0",
"version": "15.0.0",
mcollina marked this conversation as resolved.
Show resolved Hide resolved
"description": "Fastify GraphQL adapter with subscription support",
"main": "index.js",
"types": "index.d.ts",
Expand Down
Loading