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

Error message update #63

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Error message update #63

wants to merge 8 commits into from

Conversation

karamba228
Copy link

No description provided.

Copy link

vercel bot commented Feb 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
score-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 17, 2025 3:38pm

Copy link
Contributor

@srossross srossross left a comment

Choose a reason for hiding this comment

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

  1. Can this come from the backend? the response should be a 404 from the fetchScore
  2. please don;t use regex for error handling, use custom error classes

Comment on lines 108 to 110
if (!(ecosystem in ({} as EcosystemDestination))) {
throw new Error(`Invalid ecosystem: "${ecosystem}".`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please instead check the return value of the fetch when it is not a 200 for the error field

Copy link
Author

Choose a reason for hiding this comment

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

I tried this approach and while it works fine in dev it does not work in prod. React has a security feature that omits showing the actual error message, instead of the error message you will get
An error occurred in the Server Components render. The specific message is omitted in production builds to avoid leaking sensitive details. A digest property is included in this error instance which may provide additional details about the nature of the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please look at the nextjs docs for handling server errors with error.js - https://nextjs.org/docs/app/api-reference/file-conventions/error#learn-more-about-error-handling

it looks like the functionality is limited to prevent leaking server data in production.

I would instead return { status: "ecosystem_not_found" } not throw an error

then in the page server component you can do

if (status === "not_found") {
      notFound();
}

if (status === "ecosystem_not_found") {
      return ... custom component to tell the user 
}

@karamba228 karamba228 marked this pull request as draft February 7, 2025 23:16
@srossross
Copy link
Contributor

srossross commented Feb 12, 2025

also, for your own education - please look into if (!(ecosystem in ({} as EcosystemDestination))) { this expression, this is quite flawed this is the equivalent of writing if "foo" in {} which will never be true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants