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

fix(ws): Subscriptions async iterator completes and better error handling #1841

Merged
merged 8 commits into from
Apr 21, 2021
Merged

fix(ws): Subscriptions async iterator completes and better error handling #1841

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8b5b8d7
fix: simple sub async iterator with better error handling
enisdenjo Apr 19, 2021
bf7a61a
fix: typings
enisdenjo Apr 19, 2021
b1bd910
refactor: trim close event message for nicer display
enisdenjo Apr 19, 2021
bd5b919
refactor: update and use @n1ru4l/push-pull-async-iterable-iterator
enisdenjo Apr 19, 2021
8837427
chore: patch
enisdenjo Apr 19, 2021
4ed8102
fix: gql error map in else
enisdenjo Apr 19, 2021
b73283a
refactor: update @n1ru4l/push-pull-async-iterable-iterator
enisdenjo Apr 19, 2021
933444a
chore: clearer patch
enisdenjo Apr 19, 2021
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
58 changes: 53 additions & 5 deletions packages/graphiql-toolkit/src/create-fetcher/lib.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DocumentNode, visit } from 'graphql';
import { DocumentNode, visit, GraphQLError } from 'graphql';
import { meros } from 'meros';
import { createClient, Client, ClientOptions } from 'graphql-ws';
import { SubscriptionClient } from 'subscriptions-transport-ws';
Expand Down Expand Up @@ -92,10 +92,58 @@ export const createWebsocketsFetcherFromUrl = (
*/
export const createWebsocketsFetcherFromClient = (wsClient: Client) => (
graphQLParams: FetcherParams,
) =>
makeAsyncIterableIteratorFromSink<FetcherResult>(sink =>
wsClient!.subscribe(graphQLParams, sink),
);
): AsyncIterableIterator<FetcherResult> => {
let deferred: {
resolve: (done: boolean) => void;
reject: (err: unknown) => void;
} | null = null;
const pending: FetcherResult[] = [];
let throwMe: unknown = null,
done = false;
const dispose = wsClient.subscribe<FetcherResult>(graphQLParams, {
next: data => {
pending.push(data);
deferred?.resolve(false);
},
error: err => {
if (err instanceof Error) {
throwMe = err;
} else if (err instanceof CloseEvent) {
throwMe = new Error(
`Socket closed with event ${err.code} ${err.reason || ''}`.trim(),
);
} else {
throwMe = new Error(
(err as GraphQLError[]).map(({ message }) => message).join(', '),
);
}
deferred?.reject(throwMe);
},
complete: () => {
done = true;
deferred?.resolve(true);
},
});
return {
[Symbol.asyncIterator]() {
Copy link
Collaborator

@n1ru4l n1ru4l Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Symbol.asyncIterator is not available in Safari 14 (iOS), that is why I create push-pull-async-iterable-iterator in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only reason why I replaced it is #1827 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't the transpilation be handled by the user? If you'd like to support Safari 14 on iOS, simply polyfill the async iterator with Babel on build step. @acao what is your take on this?

Copy link
Collaborator

@n1ru4l n1ru4l Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong feelings about this. For myself, I can say that I am shipping an app with Safari 14 support and try to avoid babel transforms where possible, that is why I created the lib, later I had the need to transform sinks into AsyncIterables as they are easier to map/filter etc so I added it to the library. If GraphiQL does not need to rely on push-pull-async-iterable-iterator I am totally fine with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted back to using the push-pull-async-iterable-iterator following v2.1.3.

return this;
},
async next() {
if (done) return { done: true, value: undefined };
if (throwMe) throw throwMe;
if (pending.length) return { value: pending.shift()! };
return (await new Promise<boolean>(
(resolve, reject) => (deferred = { resolve, reject }),
))
? { done: true, value: undefined }
: { value: pending.shift()! };
},
async return() {
dispose();
return { done: true, value: undefined };
},
};
};

export const createLegacyWebsocketsFetcher = (
legacyWsClient: SubscriptionClient,
Expand Down