Shopify / shopify-api-js

Shopify Admin API Library for Node. Accelerate development with support for authentication, graphql proxy, webhooks
MIT License
944 stars 390 forks source link

Fix error handler in graphql client #1330

Closed 5n00p4eg closed 7 months ago

5n00p4eg commented 7 months ago

WHY are these changes introduced?

In some cases, fetch response is not available for error handling, so type error appears:

TypeError: Cannot read properties of undefined (reading 'headers')

Related issues: Shopify/shopify-app-js#764

WHAT is this pull request doing?

In case of response object is not avaliabe GraphqlQueryError is raised directly with outer response data.

Type of change

Checklist

paulomarg commented 7 months ago

Thank you so much for contributing, we appreciate it! I think this is the right idea, but I have some thoughts:

  1. There are a few other places that use this same logic, we should probably not assume we get a response in all of them. I think it would be better to have this code inside of throwFailedRequest. We could make response accept Response | undefined and then throw first thing if no response is present. That way all of these cases would behave the same way.
  2. We should have a test for this case, at least for the admin and storefront clients - you can use queueError instead of queueMockResponse in the test file to trigger this scenario
5n00p4eg commented 7 months ago

@paulomarg Take a look, please