POC of graphiql integration with bad queries

[frehner]

POC of how the storefront.query and storefront.mutate could potentially provide a link to your local GraphiQL instance with the full query pre-filled, so you can maybe more easily see the error and play around with the query?

graphiql-errors.mov

[duncan-fairley]

POC of how the storefront.query and storefront.mutate could potentially provide a link to your local GraphiQL instance with the full query pre-filled, so you can maybe more easily see the error and play around with the query?

graphiql-errors.mov

Truly next level. Simple / elegant. It's often not a trivial task to even trace an error back to a specific query as it stands. This will be very helpful.

[blittle]

Are we okay with these being logged in production? Is there a better way to output errors in production?

[frandiox]

See #923 (comment) :)

[frehner]

Agreed, I'm not sure if logging this url in production is something that would be wanted or not.

[frandiox]

Awesome 🎉
This is kind of related to #876 so I've left a comment below (feel free to take that ticket if you want).

The CLI can basically stub console.error and pick the query information to print a link to GraphiQL 👍
Also, we could parse the query there and clearly show the query name, etc 🤔

[frandiox]

Since storefront.ts doesn't have access to the local domain or port, what about simply adding the query/variables information to error.cause here, and then picking it up at the CLI level? (around lib/mini-oxygen.ts)

Suggested change

	const reqId = response.headers.get('x-request-id');
	const reqIdMessage = reqId ? ` - Request ID: ${reqId}` : '';
	let graphiqlUrl = '';
	if (query) {
	graphiqlUrl = `\n\nTry running this query in isolotion to debug: http://localhost:3000/graphiql?query=${encodeURIComponent(
	query,
	)}${variables ? `&variables=${encodeURIComponent(variables)}` : ''}\n`;
	}
	if (errors) {
	const errorMessages =
	typeof errors === 'string'
	? errors
	: errors.map((error) => error.message).join('\n');
	throw new ErrorConstructor(errorMessages + reqIdMessage);
	throw new ErrorConstructor(errorMessages + reqIdMessage + graphiqlUrl);
	}
	throw new ErrorConstructor(
	`API response error: ${response.status}` + reqIdMessage,
	`API response error: ${response.status}` + reqIdMessage + graphiqlUrl,
	);
	const reqId = response.headers.get('x-request-id');
	const errorMessage =
	(typeof errors === 'string'
	? errors
	: errors?.map?.((error) => error.message).join('\n')) ||
	`API response error: ${response.status}`;
	throw new ErrorConstructor(
	errorMessage + (reqId ? ` - Request ID: ${reqId}` : ''),
	{
	cause: {
	errors,
	reqId,
	...(process.env.NODE_ENV === 'development' && {query, variables}),
	},
	},
	);

Note: this only adds query/variables in development. Not sure if this would be useful in prod as well?

[frehner]

Since storefront.ts doesn't have access to the local domain or port, what about simply adding the query/variables information to error.cause here, and then picking it up at the CLI level?

I'm cool with that.

...(process.env.NODE_ENV === 'development' && {query, variables}),

Hmm, I can never remember, but it seems like we can't use process.env for these checks, right? Doesn't mini-oxygen not like that?

[blittle]

TIL { ...(false) } evaluates to {}

[blittle]

I still think there's room to improve how errors are logged in production. With something as simple as an invalid gql query, all that shows up in prod is:

Lately I've been trying to help multiple merchants debug pages that are 500 erroring, and it's really difficult with the default setup.

[blittle]

I mean, to get something useful in the production logs, everyone has to manually try catch their queries and mutations, which is really painful:

  let shop, product;
  try {
    const resp = await context.storefront.query<{
      product: ProductType & {selectedVariant?: ProductVariant};
      shop: Shop;
    }>(PRODUCT_QUERY, {
      variables: {
        handle: productHandle,
        selectedOptions,
        country: context.storefront.i18n.country,
        language: context.storefront.i18n.language,
      },
    });

    shop = resp.shop;
    product = resp.product;
  } catch (e: any) {
    console.log(e.stack);
    throw e;
  }

What if by default we also console.error(error.stack) or something before throwing? And allow people to disable that by a config option when creating the client?

[frandiox]

Hmm, I can never remember, but it seems like we can't use process.env for these checks, right? Doesn't mini-oxygen not like that?

We should be able to, it's replaced with true or false at build time and tree-shaked accordingly. Then, we import prod or dev bundles in the app depending on remix.config.js#serverConditions: ['worker', process.env.NODE_ENV].

(I think this should work?)

TIL { ...(false) } evaluates to {}

It might be more similar to {...undefined} but yeah, it doesn't add anything to the object.

Actually, thinking about this more, and specifically in the context of what Bret and I are talking about above - this solution would require devs to not handle errors in their ErrorBoundary, right? If they do handle the error (and decide to not log it), that would mean that it wouldn't be logged to their server terminal, right?

That's correct but, if a dev decides not to log an error in the server/terminal... isn't that a strong enough indication that they might not want to print error-related information like the faulty query as well?

Plus, it means they don't get error visibility/metrics in production either... so wouldn't it be more common that the developer logs errors at some level? (Meaning, is not logging errors a common thing?)

Yeah, I'm just feeling sad about all the existing merchants that have routes without ErrorBoundaries. Our skeleton template has proper error boundaries, but our demo store doesn't yet. Maybe I'll go add them to the demo store

But the errors are logged automatically if there are no ErrorBoundaries, right?
The problem would be when adding an ErrorBoundary that doesn't log the error in the server on purpose. But as mentioned above, that would be a strong signal that they don't want error-related information logged either.

Additionally, it seems that Remix messes around with errors (@blittle is exploring this) which potentially also means that maybe we can't go forward with this idea?

Ugh, not sure about this. I tested getting the error.cause from the CLI after Remix handlers and it was working, but no idea if there are situations where this is not true.

In prod, it looks like errors are intentionally sanitized:

Isn't this only for errors returned to the browser? 🤔 -- we wouldn't want to leak server query information anyway.

---

Since @shopify/hydrogen doesn't really know about the dev server, I think we should not print the link to GraphiQL there (what if we start the dev server on a different port or proxy domain?).
Plus, it doesn't have access to terminal components (link, color, boxes).

So I guess we need to figure out a way to get this information from storefront.query to the CLI? Either via error.cause, or a pattern in error.message, or directly by logging everything in console.debug(...) from storefront.query and hooking into that?

I wonder if Oxygen could leverage error.cause in production as well 🤔

[frandiox]

@frehner

Related to this... I've seen our skeleton routes add ErrorBoundaries like this:

  if (isRouteErrorResponse(error)) {
    console.error(error.status, error.statusText, error.data);
    return <div>Route Error</div>;
  } else {
    console.error((error as Error).message);
    return <div>Thrown Error</div>;
  }

Which wouldn't work with the approach I'm proposing about error.cause.

But even when forgetting about this use case, shouldn't we at least print the stack trace?
We do that for v1 but not for the v2 ErrorBoundary.

[frehner]

Thinking about this, we essentially need to be able to handle 3 different situations and ensure that all of them still manage to get to the CLI somehow, then, right?

1. An error is thrown, there's no boundary
2. An error is handled in the ErrorBoundary and not logged
3. An error is handled in the ErrorBoundary and is logged

And then also perhaps having different logging levels for different environments, e.g. "prod logs all errors no matter what", "prod doesn't log info ever", etc.

---

Should we consider bringing back the log utility that we had in v1? That way it's an explicit contract between us and the developer, instead of us implicitly just looking at console.log/console.error/etc.?

It would allow us to hook the cli into the calls there more easily, and also allow the dev to choose the level of logging per environment?

Thoughts? Other solutions?

[blittle]

That's correct but, if a dev decides not to log an error in the server/terminal... isn't that a strong enough indication that they might not want to print error-related information like the faulty query as well?

We just don't have a good happy path at the moment. If someone starts a new app and bases it on the demostore, if they have an error in the loader (due to a bad query or whatever), it doesn't show up anywhere other than a generic GET 500 render /products/the-full-stack. What should the best practice be?

Isn't this only for errors returned to the browser? 🤔 -- we wouldn't want to leak server query information anyway.

Not just client-side, server-side as well. If I want to catch all errors on the server (from loaders, SSR, etc), where would I put that logic?

[blittle]

Thinking about this, we essentially need to be able to handle 3 different situations

I'm not sure we need to handle all these scenarios, rather we need to make sure we are really clear about the best practice, and implement it everywhere.