Skip to content

chore(http/file_server): remove unnecessary Date header generation#3364

Merged
kt3k merged 2 commits intodenoland:mainfrom
ayame113:file-server-header
May 6, 2023
Merged

chore(http/file_server): remove unnecessary Date header generation#3364
kt3k merged 2 commits intodenoland:mainfrom
ayame113:file-server-header

Conversation

@ayame113
Copy link
Contributor

@ayame113 ayame113 commented May 5, 2023

part of #3361

As far as I know, when this code was first introduced we had to manually set the Date header. The CLI now automatically sets the Date header, so we no longer need to manually set the Date header.

There is one more place to set the Date header (link), but I left it that way because it seemed meaningful.

Also renamed setBaseHeader to createBaseHeader as I thought it would be more appropriate.


I apologize for submitting the PR all at once. Feel free to close these PRs if you don't think you need them.

@ayame113 ayame113 requested a review from kt3k as a code owner May 5, 2023 18:25
@iuioiua
Copy link
Contributor

iuioiua commented May 5, 2023

The CLI now automatically sets the Date header, so we no longer need to manually set the Date header.

Is there anywhere that references this fact?

@kt3k
Copy link
Member

kt3k commented May 6, 2023

Is there anywhere that references this fact?

The Date header should be set by hyper as it's mandatory by HTTP spec.

https://www.rfc-editor.org/rfc/rfc9110.html#section-6.6.1-6

An origin server with a clock (as defined in Section 5.6.7) MUST generate a Date header field in all 2xx (Successful), 3xx (Redirection), and 4xx (Client Error) responses

(I couldn't find exact code, but I found this issue hyperium/hyper#2998

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

@kt3k kt3k merged commit c9af841 into denoland:main May 6, 2023
@ayame113 ayame113 deleted the file-server-header branch May 6, 2023 10:18
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