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.

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

Defer doesn't work with multipart/mixed or text/event-stream #2069

Open
fredericbirke opened this issue Sep 5, 2023 · 14 comments
Open

Defer doesn't work with multipart/mixed or text/event-stream #2069

fredericbirke opened this issue Sep 5, 2023 · 14 comments

Comments

@fredericbirke
Copy link

fredericbirke commented Sep 5, 2023

Describe the bug

Schema:

schema {
  query: Query
}

type Query {
  loadFoo: Foo!
}

type Foo {
  name: String!
}


"The `@defer` directive may be provided for fragment spreads and inline fragments to inform the executor to delay the execution of the current fragment to indicate deprioritization of the current fragment. A query with `@defer` directive will cause the request to potentially return multiple responses, where non-deferred data is delivered in the initial response and data deferred is delivered in a subsequent response. `@include` and `@skip` take precedence over `@defer`."
directive @defer(
  "Deferred when true."
  if: Boolean,
  "If this argument label has a value other than null, it will be passed on to the result of this defer directive. This label is intended to give client applications a way to identify to which fragment a deferred result belongs to."
  label: String
) on FRAGMENT_SPREAD | INLINE_FRAGMENT

I use @graphql-codegen/typescript-apollo-angular for codegen.
When using this query:

query foos {
  loadFoo {
    ... @defer {
      name
    }
  }
}

, generate code (which works fine) and then try to run it, I get the following error:

ERROR ApolloError: Http failure during parsing for http://localhost:4200/graphql
    at new ApolloError (index.js:28:28)
    at QueryManager.js:673:19
    at both (asyncMap.js:16:53)
    at asyncMap.js:9:72
    at new ZoneAwarePromise (zone.js:1411:21)
    at Object.then (asyncMap.js:9:24)
    at Object.error (asyncMap.js:18:26)
    at notifySubscription (module.js:137:18)
    at onNotify (module.js:176:3)
    at SubscriptionObserver.error (module.js:229:5)

While digging deeper, you can find another error message:

{
    "headers": {
        "normalizedNames": {},
        "lazyUpdate": null
    },
    "status": 200,
    "statusText": "OK",
    "url": "http://localhost:4200/graphql",
    "ok": false,
    "name": "HttpErrorResponse",
    "message": "Http failure during parsing for http://localhost:4200/graphql",
    "error": {
        "error": {
           "message": "No number after minus sign in JSON at position 3"
           "stack: "SyntaxError: No number after minus sign in JSON at position 3
    at JSON.parse (<anonymous>)
    at XMLHttpRequest.onLoad (http://localhost:4200/vendor.js:22534:41)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:171)
    at http://localhost:4200/vendor.js:34143:49
    at AsyncStackTaggingZoneSpec.onInvokeTask (http://localhost:4200/vendor.js:34143:30)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:54)
    at Object.onInvokeTask (http://localhost:4200/vendor.js:34454:25)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:54)
    at Zone.runTask (http://localhost:4200/polyfills.js:8037:37)
    at ZoneTask.invokeTask [as invoke] (http://localhost:4200/polyfills.js:8312:26)"
         },
        "text": "\r\n---\r\nContent-Type: application/json; charset=utf-8\r\n\r\n{\"data\":{\"loadFoo\":{\"__typename\":\"Foo\"}},\"hasNext\":true}\r\n---\r\nContent-Type: application/json; charset=utf-8\r\n\r\n{\"incremental\":[{\"data\":{\"name\":\"Bar\",\"__typename\":\"Foo\"},\"path\":[\"loadFoo\"]}],\"hasNext\":false}\r\n-----\r\n"
    }
}

Which might make sense, because the response is incremental. The Backends response looks like this (this is the multipart/mixed format):

---
Content-Type: application/json; charset=utf-8

{"data":{"loadFoo":{"__typename":"Foo"}},"hasNext":true}
---
Content-Type: application/json; charset=utf-8

{"incremental":[{"data":{"name":"Bar","__typename":"Foo"},"path":["loadFoo"]}],"hasNext":false}
-----

If I try to use text/event-stream the backend anwers like this:

event: next
data: {"data":{"sanctuaryById":{"kennels":[{"id":"ccce9657-fec8-4ce0-9d7e-0160e9cec514","name":"Test","__typename":"Kennel"}],"__typename":"Sanctuary"}},"hasNext":true}

event: next
data: {"incremental":[{"data":{"name":"Dingens","__typename":"Sanctuary"},"path":["sanctuaryById"]}],"hasNext":false}

event: complete

but then the Apollo Error looks like this:

{
    "headers": {
        "normalizedNames": {},
        "lazyUpdate": null
    },
    "status": 200,
    "statusText": "OK",
    "url": "http://localhost:4200/graphql",
    "ok": false,
    "name": "HttpErrorResponse",
    "message": "Http failure during parsing for http://localhost:4200/graphql",
    "error": {
        "error": {
          "message": "Unexpected token 'e', "event: nex"... is not valid JSON"
          "stack":"SyntaxError: Unexpected token 'e', "event: nex"... is not valid JSON
    at JSON.parse (<anonymous>)
    at XMLHttpRequest.onLoad (http://localhost:4200/vendor.js:22534:41)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:171)
    at http://localhost:4200/vendor.js:34143:49
    at AsyncStackTaggingZoneSpec.onInvokeTask (http://localhost:4200/vendor.js:34143:30)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:54)
    at Object.onInvokeTask (http://localhost:4200/vendor.js:34454:25)
    at _ZoneDelegate.invokeTask (http://localhost:4200/polyfills.js:8235:54)
    at Zone.runTask (http://localhost:4200/polyfills.js:8037:37)
    at ZoneTask.invokeTask [as invoke] (http://localhost:4200/polyfills.js:8312:26)"
        },
        "text": "event: next\ndata: {\"data\":{\"loadFoo\":{\"__typename\":\"Foo\"}},\"hasNext\":true}\n\nevent: next\ndata: {\"incremental\":[{\"data\":{\"name\":\"Bar\",\"__typename\":\"Foo\"},\"path\":[\"loadFoo\"]}],\"hasNext\":false}\n\nevent: complete\n\n"
    }
}

I tested this with the react-client and it simply works. Maybe it's a bug in the codegen? I didn't use Codegen in the react test.

To Reproduce

Create a simple Backend with HotChocolate https://chillicream.com/docs/hotchocolate/v13 and defer any field. It doesn't work with the simplest case. If you want I can provide you one.

Expected behavior

Defer should work.

Environment:

├── @angular/[email protected]
├── @angular/[email protected]
├── @apollo/[email protected]
├── [email protected]
├── [email protected]
└── [email protected]
    "@graphql-codegen/add": "^5.0.0",
    "@graphql-codegen/cli": "^5.0.0",
    "@graphql-codegen/typescript-apollo-angular": "^3.5.6",
    "@graphql-codegen/typescript-operations": "^4.0.1",

Additional context

@alessbell
Copy link
Contributor

Hi @fredericbirke 👋 I work on Apollo Client and just came across this issue - would you be able to share a Replay recording of this issue? (Note: you don't have to grant Replay access to a private repo to create a recording, but it will contain source maps for your JavaScript code and record all network requests). You can share it directly with me at [email protected].

I'd love to look into this and see what may be happening. Thanks :)

@fredericbirke
Copy link
Author

Hey @alessbell,
Sorry for the late reply, I was on vacation the last few weeks.
I will create a reproduction repo and share a replay the next few days :)

@alessbell
Copy link
Contributor

Hey @fredericbirke 👋 Thanks for the offer, but I just noticed that Apollo Angular has its own HttpLink which does not yet have @defer support, so I'm guessing that explains the error you're seeing :) I'd love to help fix that.

@fredericbirke
Copy link
Author

Hey @alessbell,
Sorry for the absence. That sounds awesome! Do you still need my reproduction repo or can I help you somehow to add the defer support?

@PowerKiKi
Copy link
Collaborator

I won't work on this myself, but if you guys come up with a PR I'll gladly merge it.

@alessbell
Copy link
Contributor

I'm on vacation this week but I don't need your reproduction, @fredericbirke, but thanks for the offer :) I can pick this up when I'm back online next week.

@alessbell
Copy link
Contributor

alessbell commented Nov 1, 2023

Hey everyone 👋 I have some updates to share from looking into this today…

First, I didn’t know when or why Apollo Angular’s HttpLink diverged from Apollo Client's, so I started there. I saw it supports file uploads, which the original HttpLink doesn’t, but the larger reason this implementation was created, imo, is that Angular still uses XHR instead of fetch.

Indeed, in Apollo Angular’s http/src/utils.tsfetch” 😄 function (used by its HttpLink), the network request is made via httpClient.request. That httpClient is Angular’s HttpClientModule.

The bad news

XHR doesn’t support text streaming which is needed to power the multipart response format used to deliver the chunks. This is also an issue for React Native which uses XHR and requires a whole host of polyfills in order to get a feature like @defer working.

The good news

That said, as far as I can tell there’s nothing preventing us from using Apollo Client’s HttpLink, and by extension fetch. I created a demo doing exactly this, and it works nicely with @defer (or multipart subscriptions, for that matter): https://github.com/alessbell/simple-apollo-angular

If you're interested in trying it out yourself, the important bits are in src/app/graphql.module.ts where the provider deps contain our InjectableHttpLink which creates an Apollo Client HttpLink instead of using the HttpLink exported by Apollo Angular.

All that said, I’m not an Angular dev and would love to hear from folks who try this out. There may be other implications of not using Angular’s HttpClientModule I’m not aware of. The two I'm thinking of are retries and errors: maybe you can’t use RxJS’s retry the same way, but I haven’t tried this out with both HttpLinks for comparison so I'm not sure. And I’m not familiar with the error handling conventions in Angular: using Apollo Client’s HttpLink, I was able to receive errors by passing e.g. catchError(this.handleError) as the final argument to pipe, which receives the ApolloError, but I don’t know enough Angular to update the UI :)

Again, I’d love to hear from folks who try this out! I’m hoping it’s a feasible alternative, and it seems promising so far. As for this issue, it can probably be closed out as first party support in Apollo Angular’s HttpLink is a non-starter until Angular’s HTTP Client uses fetch.

@PowerKiKi
Copy link
Collaborator

Thank you for investigating.

The bad news

Not using HttpClientModule to support defer is unfortunately a non-starter for this lib. Because HttpClientModule offers a tight integration into Angular ecosystem. Especially HttpInterceptor which allows to intercept any XHR globally in the entire application, be it a GraphQL XHR or something else. This is typically useful to show some kind of "network activity" indicator to end-users, but it's also useful to handle errors globally.

The good news

fetch support has been introduced in Angular 16.1 via angular/angular#50247, and was made stable very recently. So it will land in Angular 17.0.

If we can leverage this new FetchBackend to support defer, then we could support defer in this lib. And either requires to use FetchBackend, or detect the backend and support defer only if FetchBackend is used.

@alessbell
Copy link
Contributor

alessbell commented Nov 2, 2023

Not using HttpClientModule to support defer is unfortunately a non-starter for this lib.

I didn't suggest removing HttpClientModule :) My point was that if anyone wants to do this today because they don't need what HttpClientModule provides, they can.

fetch support has been introduced in Angular 16.1 via angular/angular#50247, and was angular/angular#52197. So it will land in Angular 17.0.

Indeed, by setting the providers with provideHttpClient(withFetch()), HttpClientModule uses fetch with Angular >= 16.1. But unfortunately there's a blocker: Angular doesn't expose the ReadableStream which is necessary for reading individual chunks as they arrive. Instead, it assembles all chunks into a single result which the observable returns: https://github.com/angular/angular/blob/f615f4f90d3a053de2f273f6bd6d3e00ebfbb790/packages/common/http/src/fetch.ts#L115-L143

@defer, or any feature using the multipart/mixed response format, will not be possible with HttpClientModule until it exposes this functionality.

@alessbell
Copy link
Contributor

Created a feature request here: angular/angular#52494

@alessbell
Copy link
Contributor

Hi @PowerKiKi 👋 Unfortunately I haven't had much luck with my feature request.

We've gotten a fair number of support requests from users trying to use @defer with apollo-angular, and I fear this issue isn't the first place blocked users will look. Would you consider adding some language to the docs that describes the current @defer limitations and available workarounds? I'd be happy to draft the PR, thanks!

@PowerKiKi
Copy link
Collaborator

You might have a better position to update the docs as you see fit, since I am not super familiar with @defer. Please create a PR when you have time.

@DanielRose
Copy link

I found this issue when I was trying to implement support for batching requests to a Hot Chocolate backend. The error seen in the browser is the same. However, the Apollo Client currently does not support this either; see apollographql/apollo-feature-requests#387

@thegreatdane6
Copy link

I am experiencing the very same issue as @DanielRose when trying to batch with hot chocolate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants