A couple of issues/comments/suggestions

[helfer]

Hi @drcallaway 👋 nice repo!

I ran across this when trying to help some other people, and I noticed a couple of things that I think should be fixed/improved, especially since this is a link that seems to be used by a decent number of people.

1. Using an abort controller and reaching into the fetch request breaks the link abstraction. One link shouldn't be concerned with and/or reach into the internals of another link (in this case the http link). Doing so violates assumptions made by other links. The proper way for an observable to cancel a request is to simply unsubscribe and let the cancellation propagate through the chain in an orderly fashion. apollo-link-http already aborts the request if you unsubscribe from it, so nothing more should be needed. If it doesn't work as expected, then it's a bug that should be fixed in apollo-link-http.
2. Bug: Exposing a timeoutRef violates abstraction boundaries in a similar way as Timeout triggers for websocket subscriptions aswell #1. This time the timeout link is the one being reached into, leaving other subscribers between the timeout link and whoever reached into it high and dry (they never complete, nor do they ever get a TimeoutError). Whoever is using timeoutRef should just unsubscribe instead of reaching in.
3. Suggestion: Add tests for standard link functionality. Currently this link doesn't tests that it properly propagates errors, properly unsubscribes, etc. There are also no
4. Suggestion: Use travis or Circle CI to run tests on every PR.
5. Minor suggestion: When not passing a timeout to the constructor, I think it would be better to set the timeout to 0 (i.e. no timeout) and not 15 seconds, because 15 seconds is a completely arbitrary value, whereas no timeout is a reasonable non-arbitrary choice.
6. Very minor suggestion: add the timeout value to the TimeoutError and its error message, eg. "Operation timed out after x ms". Not all operations will have the same timeout, and knowing immediately what the timeout was can be helpful for debugging.

Since this is a really useful package, I hope you have the time and motivation to maintain it. If not, it might also be possible to hand the package over to the Apollo maintainers, if they're inclined to look after it.

[danmaas]

@helfer do you happen to have a pull request or example code to implement your suggestion 1. above?

I just found a bug when using apollo-link-timeout together with apollo-link-retry. After any timeout error, all future retries will also fail instantly because they re-use the same AbortController attached to the operation context, which has already been "aborted" by the first timeout.

I have a hacky patch that fixes this by removing the controller in the timeout function:

      // if timeout expires before observable completes, abort call, unsubscribe, and return error
      timer = setTimeout(() => {
        if (controller) {
          controller.abort(); // abort fetch operation

          // *** remove the controller that we just aborted so it will not block retries ***
          const context = operation.getContext();
          const fetchOptions = context.fetchOptions || {};
          if(fetchOptions.controller === controller && fetchOptions.signal === controller.signal) {
             fetchOptions = { ...fetchOptions, controller: null, signal: null };
             operation.setContext({ fetchOptions });
          }
        }

        observer.error(new TimeoutError('Timeout exceeded', requestTimeout, this.statusCode));
        subscription.unsubscribe();
      }, requestTimeout);

Do you have a cleaner fix for this? Is it worth sending my hack as a PR?

[drcallaway]

Thanks @danmaas! Yes, please submit a PR for this issue. I'm afraid that I haven't been able to keep up on this repo. I'll follow @helfer's advice and see if the Apollo team would like to take over this project.

[drcallaway]

@helfer Do you have any idea who I should contact in the Apollo organization regarding them possibly taking over this repo? Given the number of downloads of this module, I think it definitely deserves someone that can maintain it and possibly implement your suggestions above. Thanks!

[danmaas]

PR sent, and kudos to @drcallaway for writing this in the first place :). It's extremely helpful in dealing with bad mobile network conditions.

[vspedr]

Thanks for writing and maintaining this package, @drcallaway! I want to use it in a project but this issue made me a little wary.
Have @helfer's concerns about abstraction been resolved since his comment?

[drcallaway]

@vspedr I haven't addressed any of @helfer's concerns and, unfortunately, I don't plan to. I'm not really maintaining this link other than occasionally updating the Apollo version. It seems to be working for most people as it is but I'm happy to accept pull requests with any fixes.