apollographql / apollo-link

:link: Interface for fetching and modifying control flow of GraphQL requests
https://www.apollographql.com/docs/link/
MIT License
1.44k stars 345 forks source link

Subscribing to an observable causes duplicate HTTP requests #298

Open joshjg opened 6 years ago

joshjg commented 6 years ago

Intended outcome: I tried following the pattern of the third example here, and composing with an HttpLink: https://www.apollographql.com/docs/link/stateless.html

Actual outcome: All queries result in duplicate http requests being sent to the server.

How to reproduce the issue:

const httpLink = new HttpLink({ uri: foo });
const errorLink = new ApolloLink((operation, forward) => {
    const observer = forward(operation);
    observer.subscribe({ error: console.error });
    return observer;
});

const link = ApolloLink.from([
    errorLink,
    httpLink
]);
jbaxleyiii commented 6 years ago

@joshjg hmm yes I think those are written incorrectly in the docs. It should be within an Observable and bound to notify it. I can update those docs, or if you feel up to it, the apollo-link-error shows the correct way to write something like that. Would you be able to update the docs matching?

nicocrm commented 6 years ago

Picking up on this older issue. It has been classified as a documentation problem, but it makes the observable that is returned from the HTTP link error-prone to use in custom links. It would be great if it could be enhanced so that the pattern that is described in the documentation is possible. Ideally it would take care to only send the request once, even if subscribed to more than once.

Is there any plan to do that, or might you accept a pull request that would add that feature to the HTTP link? Or am I missing something that in fact does require the link to send the request every time the observable is subscribed to?

evans commented 6 years ago

@nicocrm Thank you for picking this up! Take a look at #364. At least for batching, we will support sending the request only once per batch regardless of the number of subscribers(see here).

I totally agree that we should only send the request once for subscriptions that arrive in time(before and during the fetch call). We'll probably have to make some changes to how we deal with the result of the fetch call.

The root cause is due to fact that the Observable's constructor callback is invoked on every subscribe(non-obvious/intuitive in my opinion). To solve the issue, we need to queue the observers and notify them of the fetch result, then empty the queue, in case new subscribers are added after the result arrives. We would have to worry about deleting the list when unsubscribing here. It seems like an awesome change and really needed for single requests. To answer your questions, we would love to accept a pull request. Could you make one?

naddeoa commented 5 years ago

I'm hitting this now on a React Native app. I'm seeing duplicate network requests being fired through the Android profiler. Looking over the conversation here I'm actually not sure if I can change something on my side to fix this or not. What is the solution here? We've got multiple links (context, custom metrics, http) and I'm not sure if this is a side effect of composing with from or something else.

KeithGillette commented 5 years ago

I believe I'm encountering this issue in an apollo-angular application. In attempting to track in-progress mutations, I created an ApolloLink composed with ApolloLink.from which subscribes to forward.operation in the same manner as originally described by @joshjg following the example in the Stateless Links documentation.

How may I simply be notified of the fetch result instead of duplicating the original request?

nsnyder commented 4 years ago

Hey y'all, Just opened a PR to add a warning to the documentation above because this bit me on a project. Looks like all the CI checks are running, but let me know if there's anything I need to change. It would be great to at least have a warning before I start hitting servers multiple times for every request.

Best, Nathan

nsnyder commented 4 years ago

@KeithGillette The best way at present seems to be this, from the Stateless Links documentation:

return forward(operation).map((data) => {
    // "data" will be the result. Don't use subscribe at all.
    console.log(`ending request for ${operation.operationName}`);
    return data;
  })
KeithGillette commented 4 years ago

Thanks, @nsnyder. That's exactly the approach we eventually got to work in order to track in-progress mutations for our application.