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

Consider restoring didReceiveResponse-like hook (for response/error differentiation?) #109

Closed
trevor-scheer opened this issue Dec 5, 2022 · 6 comments · Fixed by #145
Closed

Comments

@trevor-scheer
Copy link
Member

This issue is mostly for seeking feedback from current users.

#107 removed the didReceiveResponse hook, which was originally implemented for the purpose of reading headers in addition to the response body. While not its original intention (which was never super well documented), it also afforded users the ability to override the standard response.ok behavior and respond in the face of errors if they chose to. Removing the hook does remove the ability to do this, but we don't know if this capability is something users currently depend on.

Generally speaking though, this issue is seeking feedback for use cases that exist for a hook like didReceiveResponse. If this sounds interesting to you, please share some details about what you would need from this hook and how it would be useful to you.

@MarkAPhillips
Copy link

MarkAPhillips commented Jan 6, 2023

This hook was being used in our application to intercept 404 requests from the APIs and return a null data object - now this is removed what is the alternative approach to globally handle specific status codes. 404s were being returned on specific GET requests but in these instances need it handled so no error is thrown as well as return a valid GraphQL response

@glasser
Copy link
Member

glasser commented Jan 7, 2023

A combination of parseBody and throwIfResponseIsError should do the trick, I think?

@trevor-scheer
Copy link
Member Author

Overriding fetch but still using the super implementation might also be interesting.

class MyDataSource extends RESTDataSource {
  override async fetch<TResult>(
    path: string,
    incomingRequest: DataSourceRequest = {},
  ) {
    return super.fetch<TResult>(path, incomingRequest).then((result) => {
      const response = result.response;

      if (!response.ok) {
        // mutate response object accordingly
      }
      return result;
    });
  }
}

@MarkAPhillips
Copy link

One additional check done previously in the didReceiveResponse was to check the actual json response to see if it contained a specific error code - e.g. const json = await response.json(); - what is the best approach now as I assume that in most of the other methods such as fetch or parseBody will not allow this to be done - you get errors such as body used already for xxxx

@MarkAPhillips
Copy link

OK think I can solve the above by extracting the content from the parsedBody arg in throwIfResponseIsError

@MarkAPhillips
Copy link

Just to advise the solution I got working was to use a combination of throwIfResponseError and override the fetch as listed above - this did require a slightly different approach to using didReceiveResponse and required additional thought as to how this would work - making it slight more complex to implement - maybe adding some additional recipes to the docs might assist further with these type of use cases

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

Successfully merging a pull request may close this issue.

3 participants