-
Notifications
You must be signed in to change notification settings - Fork 56
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
Implement GraphQL subscription #115
Implement GraphQL subscription #115
Conversation
{ | ||
message: | ||
// DIFF: 'Variable "$arg" got invalid value "meow"; Int cannot represent non-integer value: "meow"', | ||
'Variable "$arg" got invalid value "meow"; Expected type Int; Int cannot represent non-integer value: "meow"', |
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.
I think, if it is not too breaking, we should change this message to stay up to date with graphql-js
. Wdyt?
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.
Yes, we can change it.
@boopathi I have confirmed that your test case (https://gist.github.com/boopathi/885f41720f258b50a2ba67bed76581af) now works. We are covering it in this case: graphql-jit/src/__tests__/subscription.test.ts Lines 203 to 220 in 27c197c
|
@ruiaraujo Could you please approve the CI run? Thanks! |
Any updates for this PR? |
On my side, all tests are currently passing, so I'm just waiting for a review actually. |
I'm trying it out without the resolve function mentioned in my test case. It still gives me an error. Looks like when using makeExecutableSchema, a resolve function that does |
Sorry for the late reply, as I mentioned in the other PR, that pattern is not supported by import { makeExecutableSchema } from "@graphql-tools/schema";
import { parse, subscribe } from "graphql";
function isAsyncIterable<T>(val: any): val is AsyncIterableIterator<T> {
return val != null && typeof val[Symbol.asyncIterator] === "function";
}
const schema = makeExecutableSchema({
typeDefs: `
type Subscription {
nodes: Node
}
type Node {
id: ID
}
type Query {
foo: ID
}
`,
resolvers: {
Query: {
foo: () => ""
},
Subscription: {
nodes: {
async *subscribe() {
for (let i = 0; i < 3; i++) {
yield {
id: i.toString()
};
}
},
}
}
}
});
describe("subscription happy case", () => {
test("nodes and ids", async () => {
const sub = await subscribe({ document: parse(`
subscription {
nodes {
id
}
}
`) });
if (isAsyncIterable(sub)) {
let i = 0;
for await (const val of sub) {
expect(val.data?.nodes?.id).toBe((i++).toString());
}
}
});
}); |
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.
LGTM 🎉
👍 |
try { | ||
await iterator.return(); | ||
} catch (e) { | ||
/* ignore error */ |
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.
Would it be possible to extend the comment here to explicitly state why we ignore the error?
👍 |
This PR builds upon the previous one #94.
I refactor the implementation and update with recent changes in
graphql-js
.The test has also been updated to include the latest test cases from
graphql-js
using graphql/graphql-js@1bd65a3 (3 days ago). For the diff of our test cases and theirs (mainly because we don't support root resolver), see 6c8cfc6.Close #74