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

Jump to bottom

Present 404 when domain lookup not successful #705

Merged
merged 8 commits into from
Feb 12, 2025
Merged

Present 404 when domain lookup not successful #705

merged 8 commits into from
Feb 12, 2025
+1 −1

Conversation

fraxachun
Copy link
Contributor

Implements vivid-planet/comet#3300

Shows an empty page as this 404 is just a fallback for missing configurations.

manuelblum
manuelblum previously approved these changes Jan 30, 2025
View reviewed changes
nsams
nsams previously approved these changes Jan 30, 2025
View reviewed changes
@fraxachun fraxachun dismissed stale reviews from nsams and manuelblum via 03b2062 January 31, 2025 09:51
@fraxachun fraxachun requested a review from johnnyomair January 31, 2025 09:52
@fraxachun fraxachun added the recommended-change Change should be added to existing projects and be mentioned in the next migration guide label Jan 31, 2025
@johnnyomair johnnyomair removed their request for review February 4, 2025 12:48
johnnyomair
johnnyomair previously approved these changes Feb 6, 2025
View reviewed changes
@johnnyomair johnnyomair requested a review from nsams February 6, 2025 07:10
thomasdax98
thomasdax98 previously approved these changes Feb 7, 2025
View reviewed changes
manuelblum
manuelblum previously approved these changes Feb 11, 2025
View reviewed changes
dkarnutsch
dkarnutsch previously approved these changes Feb 11, 2025
View reviewed changes
@nsams
Copy link
Member

nsams commented Feb 11, 2025

I haven't approved yet, becuase we didn't decide on new NextResponse('<html><body>404 Can not resolve domain</body></html>', { status: 404 }); vs new NextResponse(null, { status: 404 });

@fraxachun
Copy link
Contributor Author

I haven't approved yet, becuase we didn't decide on new NextResponse('<html><body>404 Can not resolve domain</body></html>', { status: 404 }); vs new NextResponse(null, { status: 404 });

As of https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-p2-semantics-12#section-8.4:
"the server SHOULD include a representation containing an explanation of the error situation, and whether it is a temporary or permanent condition"

Now a html document is presented:
image

nsams
nsams previously approved these changes Feb 12, 2025
View reviewed changes
@fraxachun
Copy link
Contributor Author

Again, I have changed it to showing the failed host in the error message.

@johnnyomair johnnyomair merged commit 5b1ad23 into main Feb 12, 2025
3 checks passed
@johnnyomair johnnyomair deleted the domain-not-found branch February 12, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsams nsams nsams approved these changes

@johnnyomair johnnyomair johnnyomair approved these changes

@manuelblum manuelblum manuelblum left review comments

@dkarnutsch dkarnutsch dkarnutsch left review comments

@thomasdax98 thomasdax98 thomasdax98 left review comments

Assignees

@fraxachun fraxachun

Labels
recommended-change Change should be added to existing projects and be mentioned in the next migration guide
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fraxachun @nsams @manuelblum @dkarnutsch @thomasdax98 @johnnyomair