deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feature(http/file_server): add 404 page support

Open mooxl opened this issue 2 years ago • 16 comments
trafficstars

Adds fallback logic for a custom 404 page as discussed and fixes #3533. The PR modifies the serveDirResponse logic. Instead of immediately throwing an error when a specified file in the URL is not found, it now performs the following checks before throwing an error:

  • If fallback is set to false, an error is thrown.
  • It checks if a custom file is specified. If yes, it uses that file; otherwise, it defaults to 404.html.
  • It retrieves the file, serves it and if it is not found, an error is thrown.

mooxl avatar Aug 12 '23 09:08 mooxl

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 12 '23 09:08 CLAassistant

Noting here that it also solves https://github.com/denoland/deno_std/issues/3420.

lino-levan avatar Aug 12 '23 18:08 lino-levan

There seems to be a lot of formatting changes which makes this pretty hard to review (did you format with prettier?). Are you able to revert this?

From what I've been able to see, it looks great! Nice work.

Apologies for that! It's my first contribution to the codebase, so it's a one-time occurrence. (i hope)

You were right, the formatting problem was indeed caused by using Prettier. I've switched to deno fmt instead, and now deno fmt --check doesn't raise any complaints. However, I have noticed some occasional formatting changes where parentheses are removed or line breaks are altered. This might be due to changes in deno fmt itself?

Thank you for bringing this to my attention!

mooxl avatar Aug 13 '23 08:08 mooxl

deno fmt allows more formatting variations than prettier. So it doesn't format back to the original state unfortunately.. Probably it's better to revert back to main, and copy paste the relevant parts..

kt3k avatar Aug 14 '23 05:08 kt3k

@mooxl I think hard coded 404.html is enough for a first pass. I'm skeptical if we need to support custom 404 file path as there's no such request from the users

kt3k avatar Aug 14 '23 06:08 kt3k

@mooxl I think hard coded 404.html is enough for a first pass. I'm skeptical if we need to support custom 404 file path as there's no such request from the users

Hey @kt3k, Thank you for the prompt response! I believe we should add support for a custom 404 file to address issue #3420. I have made the necessary formatting changes in the updated PR.

mooxl avatar Aug 14 '23 07:08 mooxl

I think the case #3420 can be solved by copying index.html to 404.html.

Also file_server is historically not intended to support all use cases, but it's more for showcasing how a simple file server can be written in Deno. So it's also important file_server.ts can be read without much difficulties by curious users. With that policy in mind, we should be very careful about adding new features to file_server.ts

kt3k avatar Aug 14 '23 07:08 kt3k

Hi @kt3k, I get your point about simplicity in file_server.ts. Let's stick to the default 404.html. I'll adjust the PR accordingly. Thanks for your guidance.

mooxl avatar Aug 14 '23 07:08 mooxl

@kt3k could you take another look?

lino-levan avatar Aug 23 '23 03:08 lino-levan

@mooxl Are you able to push this forward? I'm willing to pick it up if not.

lino-levan avatar Sep 13 '23 08:09 lino-levan

Hey @lino-levan, I'm currently preparing for my exams, so I don't have time to work on this. You can go ahead with it. :)

mooxl avatar Sep 13 '23 09:09 mooxl

Hey guys, are we still happy to push this forward?

iuioiua avatar Nov 16 '23 12:11 iuioiua

Personally a bit busy atm, hoping either you our Max can push it forward.

lino-levan avatar Nov 16 '23 16:11 lino-levan

@kt3k, how would you like to proceed with this one?

iuioiua avatar Dec 11 '23 04:12 iuioiua

There doesn't seem to be much demand for this feature and this PR has gone stale. I suggest we close without merging. WDYT, @kt3k?

iuioiua avatar Dec 19 '23 21:12 iuioiua

I'm personally in favor of this.

lino-levan avatar Dec 19 '23 21:12 lino-levan

I'm closing this without merging, as this PR has gone stale. We'd be happy to look at a new PR if anyone is willing to finish this. Thank you, either way, @mooxl!

iuioiua avatar Jan 05 '24 02:01 iuioiua