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.

Sign up for GitHub

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

POC of graphiql integration with bad queries #923

Closed
wants to merge 1 commit into from
Closed

POC of graphiql integration with bad queries #923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/hydrogen/src/routing/graphiql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ export const graphiqlLoader: GraphiQLLoader = async function graphiqlLoader({
type="application/javascript"
></script>
<script>

const windowUrl = new URL(document.URL);

let query = '';
if (windowUrl.searchParams.has('query')) {
query = decodeURIComponent(windowUrl.searchParams.get('query') ?? '');
}

let variables = '';
if (windowUrl.searchParams.has('variables')) {
variables = decodeURIComponent(windowUrl.searchParams.get('variables') ?? '');
}

ReactDOM.render(
React.createElement(GraphiQL, {
fetcher: GraphiQL.createFetcher({
Expand All @@ -65,7 +78,9 @@ export const graphiqlLoader: GraphiQLLoader = async function graphiqlLoader({
}
}),
defaultEditorToolsVisibility: true,
initialTabs: [{query: '{\\n shop {\\n name\\n }\\n}'}]
initialTabs: [{query: '{\\n shop {\\n name\\n }\\n}'}],
query: query,
variables: variables
}),
document.getElementById('graphiql'),
);
Expand Down
30 changes: 26 additions & 4 deletions packages/hydrogen/src/storefront.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,12 +340,25 @@ export function createStorefrontClient<TI18n extends I18nBase>(
errors = [{message: body}];
}

throwError(response, errors);
throwError(
response,
errors,
undefined,
query,
JSON.stringify(queryVariables),
);
}

const {data, errors} = body as StorefrontApiResponse<T>;

if (errors?.length) throwError(response, errors, StorefrontApiError);
if (errors?.length)
throwError(
response,
errors,
StorefrontApiError,
query,
JSON.stringify(queryVariables),
);

return data as T;
}
Expand Down Expand Up @@ -432,20 +445,29 @@ function throwError<T>(
response: Response,
errors: StorefrontApiResponse<T>['errors'],
ErrorConstructor = Error,
query?: string,
variables?: string,
) {
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #923 (comment) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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,
);
Comment on lines 451 to 472
Copy link
Contributor

@frandiox frandiox May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@blittle blittle May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@frehner frehner May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}