[BUG] response.json is called on non-200 responses from Shopify backend

[AbdulRahmanAlHamali]

Describe the bug The code here: https://github.com/Shopify/hydrogen/blob/%40shopify/hydrogen%401.2.0/packages/hydrogen/src/hooks/useShopQuery/hooks.ts#L93

attempts to parse JSON, and when that fails, looks at the source of error. This means that if a 4xx response is received from the server with a valid JSON body, the code would assume it is a valid GraphQL response, and would proceed with it, leading to unexpected behavior.

The code should ideally check the status code before attempting to parse, and only proceed with the parsing upon confirming that the status code is 200.

To Reproduce

Direct hydrogen to a server that returns some 4xx response, e.g. 499 with a JSON body.

Expected behaviour Any non-200 response should immediately be classified as such without proceeding to the next layer

• 1.2.0

[blittle]

The challenge here is that non-200 responses often have JSON. useShopQuery returns an object. If it's a non-200 response, what should the hook return?

1. A string - now the return result of useShopQuery is complex, and someone needs to do a typeof on the outside. Sometimes it's an object, other times it's a string. See here: https://github.com/Shopify/hydrogen/blob/v1.x-2022-07/packages/hydrogen-ui/src/storefront-api-response.types.ts#L52
2. Throw an Error - If the hook throws for a non-200 response, handling that error on the outside becomes difficult with a try catch.

My thoughts are useShopQuery should always return an object. If the response isn't parsable, then we can take the response text and just put it in a { errors: "response text here" }.

That would make it so on the outside, the user can always reliably const { data, errors } = useShopQuery(...) and conditionally look to see if errors is defined.

What do you think @frehner @AbdulRahmanAlHamali?

[AbdulRahmanAlHamali]

@blittle I'm actually not aware of many endpoints that return json with non-200 codes. GQL endpoints return 200 even if they contained errors. I think the problem here is two-fold:

1. If we actually have an endpoint that returns JSON on errors, we cannot guarantee that it would return an errors object. So the code needs to account for that. We are currently treating that response as a GraphQL error, which is not necessarily correct
2. We are raising the error as we see here. I think this is what you meant and this is what is causing the problem with TypeErrors. Correct?

[blittle]

@frehner in your research did you come across non-200 JSON responses?

Let's consider all the scenarios useShopQuery() might trigger, and how it should behave:

1. Normal response

// Response data
{ "data": ... }

// Hook usage
const {data} = useShopQuery(...);

2. GraphQL error response

// Response data
{ "errors": [{"message": "", ...}] }

// Hook usage
const {data, errors} = useShopQuery(...);

I don't believe the errors property has a consistent type. Which makes it difficult for the user to know how to handle it.

3. Fetch error These are triggered by network or CORS issues. The underlying fetch throw's an error. So there is no response code.

try {
  const {data, errors} = useShopQuery(...);
  
  if (errors) {
    // Do something
  }
} catch (error) {
 // Do something
}

This is awkward, I doubt anyone ever has a try catch, so usually these problems will just bubble and cause the whole page to 500 error. If a dev wanted to gracefully handle these errors, it's hard because it's not obvious useShopQuery might throw internally, in addition to returning an errors reference.

4. Non 200 Responses

We don't know every scenario these might happen. An easy one to reproduce is to use an invalid token. In that case, there is no response body, just a 401 response code. Other times I know that we've seen HTML in non-200 responses. I recall that there was JSON in non-200 responses sometimes too, but don't remember where. If we throw on non-200 responses, it will exasperate the problem in #3. Uncaught errors in Hydrogen trigger a whole page error. What if they want to gracefully degrade, and only have a portion of the page fail?

try {
  const {data, errors} = useShopQuery(...);
  
  if (errors) {
    // Do something
  }
} catch (error) {
 // Do something
}

[frehner]

@frehner in your research did you come across non-200 JSON responses?

According to the docs (scroll down to the section 4xx and 5xx status codes) it seems like most of them are JSON.

---

I don't believe the errors property has a consistent type. Which makes it difficult for the user to know how to handle it.

If it's a pure GraphQL error, then it does have a consistent shape actually - or at least, should. If you look at FormattedExecutionType that we use here:

https://github.com/Shopify/hydrogen/blob/d043e3facdaf34c5295803da5ccb2588b96d3c7b/packages/hydrogen-ui/src/storefront-api-response.types.ts#L28-L31

It comes from graphql, and looks like this:

export interface FormattedExecutionResult<
  TData = ObjMap<unknown>,
  TExtensions = ObjMap<unknown>,
> {
  errors?: ReadonlyArray<GraphQLFormattedError>;
  data?: TData | null;
  extensions?: TExtensions;
}

In turn, the GraphQLFormattedError looks like:

/**
 * See: https://spec.graphql.org/draft/#sec-Errors
 */
export interface GraphQLFormattedError {
  /**
   * A short, human-readable summary of the problem that **SHOULD NOT** change
   * from occurrence to occurrence of the problem, except for purposes of
   * localization.
   */
  readonly message: string;
  /**
   * If an error can be associated to a particular point in the requested
   * GraphQL document, it should contain a list of locations.
   */
  readonly locations?: ReadonlyArray<SourceLocation>;
  /**
   * If an error can be associated to a particular field in the GraphQL result,
   * it _must_ contain an entry with the key `path` that details the path of
   * the response field which experienced the error. This allows clients to
   * identify whether a null result is intentional or caused by a runtime error.
   */
  readonly path?: ReadonlyArray<string | number>;
  /**
   * Reserved for implementors to extend the protocol however they see fit,
   * and hence there are no additional restrictions on its contents.
   */
  readonly extensions?: {
    [key: string]: unknown;
  };
}

Additionally, Shopify provides a couple of pre-built extensions for that error, which can be found here:

https://github.com/Shopify/hydrogen/blob/d043e3facdaf34c5295803da5ccb2588b96d3c7b/packages/hydrogen-ui/src/storefront-api-response.types.ts#L4-L19

---

Note that my understanding of all of this was just from looking at the docs and asking some questions in the SFAPI Slack channel. I did a small amount of testing (e.g. bad token / 401) to confirm where I could, but it's also hard to manually trigger all the different errors so I relied on my faith that the docs / SFAPI team were correct.

[blittle]

Thank you @frehner for more context. What do you think, should useShopQuery be throwing? Already it does for failed to fetch. But should it be throwing for non-200 responses?

[frehner]

What do you think, should useShopQuery be throwing? Already it does for failed to fetch. But should it be throwing for non-200 responses?

Hmm, I don't know. I would have to kind of defer to what the broader ecosystem does in these situations. The two most popular being swr and react-query.

It seems swr handles thrown errors and returns it as part of the error object.

And it appears react-query does the same.

So yeah, I think it would be best if Hydrogen's version also handled all errors in every situation, and just returned a {data, error} object.

[blittle]

My opinion: it's already hard enough to consistently and properly handle errors. If we force devs to handle errors in different ways, it's a lot less likely they will properly do it.

[shopify[bot]]

Oxygen deployed a preview of your fd-codegen-warning branch. Details:

Storefront	Status	Preview link	Deployment details	Last update (UTC)
custom-cart-method	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM
third-party-queries-caching	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM
vite	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM
subscriptions	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM
optimistic-cart-ui	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM
skeleton	✅ Successful (Logs)	Preview deployment	Inspect deployment	April 11, 2024 2:53 PM

Learn more about Hydrogen's GitHub integration.

[github-actions[bot]]

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset. If the changes are user-facing and should cause a version bump, run npm run changeset add to track your changes and include them in the next release CHANGELOG. If you are making simple updates to examples or documentation, you do not need to add a changeset.