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

Customising the error parser doesn't seem to work #56

Closed
penalosa opened this issue Dec 11, 2024 · 3 comments
Closed

Customising the error parser doesn't seem to work #56

penalosa opened this issue Dec 11, 2024 · 3 comments
Assignees
Labels
Type: Bug The issue has indentified a bug

Comments

@penalosa
Copy link

penalosa commented Dec 11, 2024

Package version

4.1.0-beta.2

Describe the bug

As part of upgrading Wrangler to use Youch 4 (ref: cloudflare/workers-sdk#2977 (comment)), I'm trying to replicate Youch 3's behaviour around hiding native stack frames.
The README seems to indicte youch.useTransformer() can be used to add a transformer (that would then hide certain stack frames), but it doesn't seem like that ErrorParser instance is actually used when rendering the HTML page: https://github.com/poppinss/youch/blob/4.x/src/youch.ts#L77

I can make a reproduction repo if needed, but this is roughly how I'm using Youch:

let youch = new Youch();
const headers = [...request.headers.entries()];
const cookieHeader = headers.find((h) => h[0] === "cookie");
youch.metadata.group("Request", {
  url: {
    key: "URL",
    value: request.cf?.prettyErrorOriginalUrl ?? request.url,
  },
  method: {
    key: "Method",
    value: request.method,
  },
  headers: headers
    .filter((h) => h[0] !== "cookie")
    .map((e) => ({
      key: e[0],
      value: e[1],
    })),

  // @ts-expect-error Youch is a bit strict
  cookies: cookieHeader
    ? cookieHeader[1]
        .split(";")
        .map((c) => ({ key: c.split("=")[0], value: c.split("=")[1] }))
    : undefined,
});

youch = youch.useTransformer((error, source) => {
  error.frames = error.frames.filter(
    (f) =>
      !f.fileName?.includes(".wrangler/tmp") &&
      !f.fileName?.includes("wrangler/templates/middleware"),
  );
});

// @ts-expect-error Youch uses this field to render a hint
error.hint = `<a href="https://developers.cloudflare.com/workers/" target="_blank" style="text-decoration:none">📚 Workers Docs</a>`;

return new Response(
  await youch.toHTML(error, {
    offset: 1,
    title: "Your Worker encountered an error",
  }),
  {
    status: 500,
    headers: { "Content-Type": "text/html;charset=utf-8" },
  },
);

As a secondary point—does it look like this is the right way to use this package? Our previous implementation with Youch 3 had a lot less boilerplate: https://github.com/cloudflare/workers-sdk/blob/main/packages/miniflare/src/plugins/core/errors/index.ts#L261-L272

@thetutlage
Copy link
Member

Hello @penalosa

Yup, that seems like a bug and I will work on a fix.

Regarding being boilerplate heavy. I think the metadata API is boilerplate since its more generic now and doesn't except/rely on HTTP headers directly. Do you think adding first-class support for rendering request headers automatically is something you would want?

@penalosa
Copy link
Author

Thanks!

In terms of boilerplate, while the request headers are now a bit more verbose I can totally see the reasoning for making metadata more generic—it's no big deal for us to provide that more explicitly. The two things that I think would be quite helpful are:

  • A hint parameter in the API; the current error.hint I found quite unintuitive and it doesn't play well with TypeScript
  • A toggle for showing/hiding native frames, as in Youch 3 (defaulting to hiding?). I may be looking at this wrong, but the v4 "in app" label is easy to miss, and it's hard to distinguish user code frames if there are 5 or 6 modules/native frames

In general though, this looks like a nice upgrade!

@thetutlage thetutlage added the Type: Bug The issue has indentified a bug label Dec 13, 2024
@thetutlage
Copy link
Member

Hello @penalosa

I have fixed the issue with transformers in this release. https://github.com/poppinss/youch/releases/tag/v4.1.0-beta.3

Also, added support for defining the request properties via the toHTML method and they will be added to the metadata behind the scenes. Essentially, the following should work.

await youch.toHTML(error, {
  request: {
    url: '/',
    method: 'GET',
    headers: Object.fromEntries(request.headers)
  }
})

They will be displayed as follows.

CleanShot 2024-12-13 at 10 46 58@2x


Re: Hiding native frames, I have opened an issue for it and will add support for it soon #57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug The issue has indentified a bug
Projects
None yet
Development

No branches or pull requests

2 participants