Implement subscription

[hoangvvo]

This implements subscription support via jit.subscribe.

Close #74

[hoangvvo]

As per #74 (comment), we have one failing test I don't know what is wrong:

Subscription Initialization Phase › resolves to an error for source event stream resolver errors

    expect(received).toEqual(expected)

    Difference:

    - Expected
    + Received

      Object {
        "errors": Array [
    -     Object {
    -       "locations": Array [
    -         Object {
    -           "column": 13,
    -           "line": 3,
    -         },
    -       ],
    -       "message": "test error",
    -       "path": Array [
    -         "importantEmail",
    -       ],
    -     },
    +     [GraphQLError: test error],
        ],
      }

cc @ruiaraujo

[ruiaraujo]

@hoangvvo apologies for not reacting earlier.

Do you plan to finish this PR?

[hoangvvo]

@hoangvvo apologies for not reacting earlier.

Do you plan to finish this PR?

Hey, no worry. I do plan to finish the PR. I will get back to this once I have a chance sometimes later this week.

[hoangvvo]

@ruiaraujo For the failing test, if I use jest.toMatchObject instead of jest.toEqual, it shows the error more clearly:

Subscription Initialization Phase › resolves to an error for source event stream resolver errors

    expect(received).toMatchObject(expected)

    Expected value to match object:
      {"errors": [{"locations": [{"column": 13, "line": 3}], "message": "test error", "path": ["importantEmail"]}]}
    Received:
      {"errors": [[GraphQLError: test error]]}
    Difference:
    - Expected
    + Received

    @@ -1,11 +1,11 @@
      Object {
        "errors": Array [
          Object {
            "locations": Array [
              Object {
    -           "column": 13,
    +           "column": 11,
                "line": 3,
              },
            ],
            "message": "test error",
            "path": Array [

      545 |       });
      546 | 
    > 547 |       expect(result).toMatchObject({
          |                      ^
      548 |         errors: [
      549 |           {
      550 |             message: "test error",

      at testReportsError (src/__tests__/subscription.test.ts:547:22)
      at Object.<anonymous> (src/__tests__/subscription.test.ts:516:5)

It seems like column is slightly off. Is this a problem?

The relevant code is at

graphql-jit/src/execution.ts

Line 1762 in 7937b3b

throw locatedError(error, fieldNodes, pathToArray(responsePath));

[ruiaraujo]

This is not supposed to be there in case of subscriptions, right?

[hoangvvo]

compiledQuery.query() will still be there. It will be used in mapSourceToResponse function similar to the way graphql-js uses execute. (with rootValue being the source)

https://github.com/graphql/graphql-js/blob/main/src/subscription/subscribe.js#L143

[hoangvvo]

Is there anything I can do to push this forward? Thanks.

[ruiaraujo]

Can you fix the build, we can get it in and then iterate on the API if needed.

[hoangvvo]

I believe all of the changes I made have been covered by tests. I am not sure why coverage is dropped below 94%, causing the build to fail.

I made the change to bump it back up. Builds are passing now.

[ruiaraujo]

👍

[ruiaraujo]

@boopathi please take a look, I intend to merge this as is and iterate on the API if needed.

[boopathi]

I was trying it locally with GraphQL-Tools' makeExecutableSchema and it required me to specify the default resolver as the identity function x => x. Let me know if I'm using it wrong.

The source code for this test is available here in this gist - https://gist.github.com/boopathi/885f41720f258b50a2ba67bed76581af

The resolver is highlighted in the gist. Is it possible to not specify this resolver explicitly?

[boopathi]

Probably an unimportant issue - the arguments to the next() fn in the async iterator is ignored in the implementation of the function mapAsyncIterator -

https://github.com/graphql/graphql-js/blob/fe9ea6db90a12c88d6372f88b61ca68e7aa3659b/src/subscription/mapAsyncIterator.js#L42-L43

I was trying to pass back a value from the GraphQL subscription result to the async generator function -

// implementation
async *subscribe() {
  let previousValue = initialValue ?? previousValueFromInput;
  for (;;) {
    previousValue = yield promise(previousValue);
  }
}

// usage
const sub = await subscribe(`subscription { value }`);
const firstValue = await sub.next(
  something, client, wants, to, send, during, iteration /* These arguments are ignored */
);

This is probably a GraphQL-JS bug, I'm just curious if there are no usages of this pattern. We do not have to resolve this comment in this PR.

[hoangvvo]

That is indeed a valid pattern. Perhaps we can implement our own version of mapAsyncIterator (maybe we should either way because mapAsyncIterator is "internal"-ish for graphql-js? Maybe we can do it in a different PR

[hoangvvo]

Yup mapAsyncIterator was changed to named export:

https://github.com/APIs-guru/graphql-js/blob/28315cd3f51b5ac212284f8583fb481053fa31a0/src/subscription/mapAsyncIterator.js graphql/graphql-js#2916

We should implement our own to avoid breaking changes like thi

[hoangvvo]

Looks like graphql-tools is also broken too for the subscribe function from graphql-js:

https://gist.github.com/hoangvvo/fa2901c5707eed116144426563fd6009#file-graphql-js-subscription-test-ts

not graphql-tools, even if I create a schema with GraphQLSchema but not setting the identity function, graphql-js fails:

https://gist.github.com/hoangvvo/fa2901c5707eed116144426563fd6009#file-graphql-js-no-tools-subscription-test-ts

[hoangvvo]

Odd because in subscription.test.ts, there are cases where resolve function is not set

[hoangvvo]

I am closing this in favor of #115.