deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feat(http/unstable): add `cleanUrls` option to `serveDir`

Open lowlighter opened this issue 1 year ago • 7 comments

This PR introduces a new cleanUrls option to serveDir function

It is inspired from a common pattern where you don't have to specify the html extension to serve a page. (e.g. deno.com/blog, github.com/denoland/std, ..., some hosters such as Vercel also propose a similar feature)

Considerations:

  • Only .html is handled by this new option, for more advanced usage user would need to use their own routing
    • The reasoning for what it'd be supported natively by std is that it's a really common use case, and it's actually not that easy to get a simple workaround (e.g. if you use route(request), as Request are immutable you'll need to do some shenanigans to trim extensions, catch the 404 to retry with it, etc.)
    • It'd be an extremely nice QOL and since it's gated behind an option would not be a breaking change and would not hinder performances of existing apps
  • If the path without extension "shadows" another file or directory, the latter takes precedences over the otherwise cleaned url aliased file

Closes #6232

lowlighter avatar Dec 04 '24 05:12 lowlighter

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.69%. Comparing base (2b6049a) to head (211da6e). Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6231      +/-   ##
==========================================
- Coverage   94.70%   94.69%   -0.01%     
==========================================
  Files         567      569       +2     
  Lines       46793    46825      +32     
  Branches     6582     6589       +7     
==========================================
+ Hits        44315    44341      +26     
- Misses       2436     2442       +6     
  Partials       42       42              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Dec 04 '24 05:12 codecov[bot]

I'm closing it for now, but I still think this patch would be a nice QOL, especially since it doesn't have that much maintenance/overhead, but this may need further discussion as pointed on the discord server since opinions seems to be diverging.


Feedback from the discord:

It's common as a "framework feature", but for file server (like apache) you usually need either rewriting urls or set the default mime type to text/html

A workaround is to change the file structure to use folder + index.html instead but although it works it makes you change the way you organize file rather than your code (which shouldn't be, the file structure may be required to be a certain way depending on your use case)

lowlighter avatar Dec 04 '24 05:12 lowlighter

Maybe I should wait for #6668 to be sorted out before continuing working on this ? @iuioiua @kt3k Especially since the other pr includes the import.meta.main entrypoint so I could support the new options from the cli tools too (should it be enabled by default ?)

lowlighter avatar May 28 '25 01:05 lowlighter

I've drafted #6668 to wait until we see what we do with https://github.com/denoland/deno/issues/29447.

iuioiua avatar May 28 '25 02:05 iuioiua

I think this is independent from #6668 (#6668 changes the entrypoint and this changes serveDir API)

I could support the new options from the cli tools too (should it be enabled by default ?)

Let's discuss that in an independent issue.

kt3k avatar May 28 '25 03:05 kt3k

Ok then I wasn't sure if you wanted to avoid conflicts on existing work then

The build fail is i think unrelated to my pr, it's failing due to rename/renameSync (both unstable api, maybe flaky tests ?)

lowlighter avatar May 28 '25 04:05 lowlighter

The CI failure seems caused by CLI canary change (probably this one https://github.com/denoland/deno/commit/cb23193f74b2cf34cf06111f9ebbf0599d5ec24d ). Fixing it in https://github.com/denoland/std/pull/6695

kt3k avatar May 29 '25 03:05 kt3k