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

feat(http): Add --header option to file_server #2977

Merged
merged 6 commits into from
Dec 27, 2022
Merged

Conversation

lino-levan
Copy link
Contributor

A big thanks to @plohm12 for the initial work in #2589.

This PR adds --cache (or -C) as an option for file_server. It follows a similar spec to http_server over on the node side where --cache=-1 means no-store.

@lino-levan lino-levan requested a review from kt3k as a code owner December 4, 2022 01:41
@timreichen
Copy link
Contributor

Is there a use case of maxAge=0? Or why is it -1 for no-store?

@lino-levan
Copy link
Contributor Author

lino-levan commented Dec 4, 2022

@timreichen good question. maxAge=0 essentially behaves something like must-revalidate. no-store is subtly different than this:

  • must-revalidate caches the response on the client, but always checks with the server to get the latest information
  • no-store NEVER caches the response on the client. This is usually for security reasons.

There may be a point to be made that there is no value in having both supported in file_server, but given that it costs essentially nothing to support both (and c=-1 is already standard in http_server) I thought that there wasn't much of a downside.

@kt3k
Copy link
Member

kt3k commented Dec 5, 2022

Adding specific option for each header feels problematic to me as there are many HTTP headers. How about adding option for appending any kind of headers by the pairs of keys and values.

@lino-levan
Copy link
Contributor Author

@kt3k 100% agree in theory, but I think this should be an addition feature. I think that caching is a very basic header that should really have been an option.

What are you thinking for cli inputs for adding arbitrary http headers?

I would imagine something like: --headers="Set-Cookie: example=hi" but I have no idea how we would parse multiple headers. Is there a standard separator that we could use (I'm pretty sure's it is newlines but we can't exactly use newlines in this situation)?

@kt3k
Copy link
Member

kt3k commented Dec 6, 2022

I would imagine something like: --headers="Set-Cookie: example=hi" but I have no idea how we would parse multiple headers. Is there a standard separator that we could use (I'm pretty sure's it is newlines but we can't exactly use newlines in this situation)?

How about following curl's -H, --header option?

--header "set-cookie: example=hi" --header "cache-control: no-cache" -H "x-custom-header: xyz"

parse of flags module has collect option, which enables the usage of the same cli arg multiple times

parse(Deno.args, { collect: ["header"] });

@lino-levan
Copy link
Contributor Author

@kt3k that makes a lot of sense. So now to move forward, should we :

  • Change this PR to add --header and remove --cache
  • Merge this PR, and then open a separate PR for --header

I'm game for either, though I personally do see value in --cache so I would lean towards option 2.

@kt3k
Copy link
Member

kt3k commented Dec 6, 2022

I prefer the first option. --cache option seems needing further design work about what ranges of values to support with it, but that kind of stuff is out of scope of file_server in my view. (One of the purposes of http/file_server is to show the example of implementing simple static file server in Deno, rather than providing a complete static file server with large number of useful options.

@lino-levan
Copy link
Contributor Author

Sounds good to me, I will rework this PR when I get the chance (probably this weekend).

@lino-levan lino-levan changed the title feat(http): Add --cache option to file_server feat(http): Add --header option to file_server Dec 10, 2022
@lino-levan
Copy link
Contributor Author

@kt3k PTAL

Thanks!

http/file_server.ts Outdated Show resolved Hide resolved
http/file_server.ts Outdated Show resolved Hide resolved
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

@kt3k kt3k merged commit 5bb2db6 into denoland:main Dec 27, 2022
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.

3 participants