Add range request and etag support to `file_server.ts` #1028

pseudosavant · 2021-07-12T20:59:31Z

First pass at adding support for range requests

Basic etag support
Server name header
Date and Last-Modified headers
Added ~200 additional mimetypes

- Basic etag support - Server name header - Date and Last-Modified headers - Added ~200 additional mimetypes

…main

CLAassistant · 2021-07-12T20:59:37Z

All committers have signed the CLA.

kitsonk · 2021-07-12T21:42:04Z

Personal opinion is that this is really getting into the range of what a Web server framework does. file_server is intended to be a minimalist example application. Also it doesn't have tests, but even with exhaustive tests I would feel the same way, that it isn't part of the std library.

- Corrected single-quotes to double-quotes - Fixed use before assignment for `start` and `end`

pseudosavant · 2021-07-12T23:16:13Z

Personal opinion is that this is really getting into the range of what a Web server framework does. file_server is intended to be a minimalist example application. Also it doesn't have tests, but even with exhaustive tests I would feel the same way, that it isn't part of the std library.

Agreed on the tests. I'm working on them. I wanted to get some feedback on this first. Is file_server.ts not actually part of the std library even though it is in the std repo?

I'm not sure how supporting a basic HTTP 1.1 feature makes it a 'framework'? Without range support, it is basically an HTTP 1.0 server. I know file_server.ts is made for basic non-prod work, but even for a basic 'built-in' web server, it is very limited. If it is to remain this limited, it should advertise itself as HTTP 1.0.

I have a video player app that I work on that does thumbnailing for any video URL. Lack of range support caused it to either: 1) only ever show the very first frame no matter the requested timestamp, or 2) it wouldn't work at all, but also wasn't throwing a visible error in the browser. The issue affected playback too. The browser video engine wouldn't seek (even fully downloaded files) or would have to download the entire file because the MP4 index was at the end.

- Set standard base headers (`server`, `accept-ranges`, `date`) for all responses - Fixed handling of malformed range requests - `response.body` is empty for 304 responses

- Added tests for: range requests, etag headers, mime-type headers, date headers - Populated test `hello.html` and `test file.txt` files with test data

…e test, contentType test - CORS has its own specific test now - Excess tests for CORS removed from other tests - Corrected test for `/testdata/%` URL - Fixed contentType test - All tests now pass

pseudosavant · 2021-07-13T19:47:42Z

I have added and fixed all the tests in file_server_ts.ts, and fixed issues in file_server.ts discovered from tests.

pseudosavant · 2021-07-14T00:12:41Z

Of course all of the tests passed on my machine, but not on the CI. The file metadata (length, date) is being returned different, probably due to platform differences. I'm changing them to dynamically pull the metadata instead of it being hard coded.

…ad of being hardcoded.

http/file_server_test.ts

http/file_server.ts

kt3k · 2021-07-14T08:26:54Z

I'm slightly in favor of including this change because according to the file's history we've been accepting the PRs which simply add the usability as a file server, (which therefore reduce the minimality as an example). I think this is one of that types of changes. But I'd like to hear other collaborators opinions as well.

pseudosavant · 2021-07-15T17:35:03Z

Fixed a couple of hardcoded values I missed in 2 tests.

ry

This falls in the scope of the file_server.ts and looks well-implemented with tests. Thank you for this PR @pseudosavant !

I'm a little concerned that the file_server now depends on createHash. Can crypto.subtle.digest be used instead?

pseudosavant · 2021-07-16T17:45:59Z

I'm a little concerned that the file_server now depends on createHash. Can crypto.subtle.digest be used instead?

Sure, that was a straight-forward change. I just pushed a commit for it.

ry · 2021-07-19T15:06:21Z

http/file_server_test.ts

+    "",
+  );
+  return hashHex;
+};


This code is duplicated... Seems better if you export it.

Also the .join("",) seems odd...

I can move that out into a module. Is there guidance on where I should put that in the repo?

As for the join. Agreed .join("",) looks odd. But do you mean the trailing comma or joining the array into strings using join? The comma and line break are being added by deno fmt. I don't understand why it is doing that though. That method only accepts 1 argument.

I pushed a commit to make createEtagHash clearer, and format better. It doesn't address moving it out into a module yet.

@ry Any guidance on where I should put the module? Should I put it in something like _createEtagHash.ts?

That would work. But I question whether the test needs the ability to create the etag header generally - how about just hardcoding some expected etag values into the test?

My first tests did used hard coded values but it didn't work. I learned that the last modified time, and the system reported file size, aren't consistent across environments. Those are the two inputs, concatenated, for createEtagHash.

kt3k · 2021-07-20T10:21:34Z

LGTM except CLA check error. It seems 2 email addresses are used in the commits and one of them causing the error... @pseudosavant Can you fix this situation?

pseudosavant · 2021-07-21T18:42:56Z

LGTM except CLA check error. It seems 2 email addresses are used in the commits and one of them causing the error... @pseudosavant Can you fix this situation?

I've been trying to get it corrected. I didn't realize my git config was slightly different on one of my other machines I used. I followed the CLA assistant's directions and added and verified the other email address in my GitHub account. The CLA assistant doesn't seem to pick it up though. And when I try doing the assistant again it says I've signed it already.

Do you know of another way I could fix this?

…e.digest`.

kt3k · 2021-07-22T06:10:21Z

Only the last commit had a different email address. I squashed it to the one before it.

kt3k

@pseudosavant Thank you for your contribution!

pseudosavant · 2021-07-23T01:29:29Z

@pseudosavant Thank you for your contribution!

Thanks for the help getting it merged in! Deno is a great project. 🙏

pseudosavant added 2 commits July 12, 2021 13:55

First pass at adding support for range requests

68f47e9

- Basic etag support - Server name header - Date and Last-Modified headers - Added ~200 additional mimetypes

Merge branch 'main' of https://github.com/pseudosavant/deno_std into …

986725f

…main

PR Fixes

be22db2

- Corrected single-quotes to double-quotes - Fixed use before assignment for `start` and `end`

pseudosavant added 5 commits July 13, 2021 10:59

Improved response headers, handling of malformed requests and 304s

3e9034f

- Set standard base headers (`server`, `accept-ranges`, `date`) for all responses - Fixed handling of malformed range requests - `response.body` is empty for 304 responses

Added tests for HTTP range request support

8d2dae7

- Added tests for: range requests, etag headers, mime-type headers, date headers - Populated test `hello.html` and `test file.txt` files with test data

Fixed incorrect definition of the testdata location

487841a

Fixed tests: removed excess CORS checks, added CORS test, bad filenam…

2e47e20

…e test, contentType test - CORS has its own specific test now - Excess tests for CORS removed from other tests - Corrected test for `/testdata/%` URL - Fixed contentType test - All tests now pass

Fixed handling of malformed URL (/testdata/%)

4ae4da0

pseudosavant added 2 commits July 13, 2021 17:22

Fixed code formatting issues

82e1a34

File metadata (size, dates) is dynamically retrieved at runtime inste…

434cde4

…ad of being hardcoded.

kt3k reviewed Jul 14, 2021

View reviewed changes

http/file_server_test.ts Outdated Show resolved Hide resolved

kt3k reviewed Jul 14, 2021

View reviewed changes

http/file_server.ts Outdated Show resolved Hide resolved

Cleaned up formatting with deno fmt

4e650dd

kitsonk requested review from bartlomieju, lucacasonato and ry July 14, 2021 21:59

pseudosavant added 2 commits July 15, 2021 10:29

Removed additional hardcoded values. Now determined at runtime.

b13b236

Corrected formatting with deno fmt.

78f0255

ry reviewed Jul 16, 2021

View reviewed changes

pseudosavant changed the title ~~First pass at adding range request support to file_server.ts~~ Add range request and etag support to file_server.ts Jul 16, 2021

ry reviewed Jul 19, 2021

View reviewed changes

Removed dependency on createHeash. Etag hash now uses `crypto.subtl…

f981f56

…e.digest`.

kt3k force-pushed the main branch from 9375dd2 to f981f56 Compare July 22, 2021 05:47

kt3k approved these changes Jul 22, 2021

View reviewed changes

kt3k merged commit 0722cd0 into denoland:main Jul 22, 2021

tjosepo mentioned this pull request Oct 28, 2021

feat(http/file_server): add streaming support, fix empty file handling #1479

Merged

fonsp mentioned this pull request May 19, 2022

file_server: Chrome DevTools Workspaces stopped working #2251

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add range request and etag support to `file_server.ts` #1028

Add range request and etag support to `file_server.ts` #1028

pseudosavant commented Jul 12, 2021

CLAassistant commented Jul 12, 2021 •

edited

Loading

kitsonk commented Jul 12, 2021

pseudosavant commented Jul 12, 2021

pseudosavant commented Jul 13, 2021

pseudosavant commented Jul 14, 2021

kt3k commented Jul 14, 2021

pseudosavant commented Jul 15, 2021

ry left a comment •

edited

Loading

pseudosavant commented Jul 16, 2021

ry Jul 19, 2021

pseudosavant Jul 20, 2021

pseudosavant Jul 20, 2021

pseudosavant Jul 21, 2021

ry Jul 21, 2021

pseudosavant Jul 21, 2021

kt3k commented Jul 20, 2021

pseudosavant commented Jul 21, 2021

kt3k commented Jul 22, 2021

kt3k left a comment

pseudosavant commented Jul 23, 2021

Add range request and etag support to file_server.ts #1028

Add range request and etag support to file_server.ts #1028

Conversation

pseudosavant commented Jul 12, 2021

CLAassistant commented Jul 12, 2021 • edited Loading

kitsonk commented Jul 12, 2021

pseudosavant commented Jul 12, 2021

pseudosavant commented Jul 13, 2021

pseudosavant commented Jul 14, 2021

kt3k commented Jul 14, 2021

pseudosavant commented Jul 15, 2021

ry left a comment • edited Loading

Choose a reason for hiding this comment

pseudosavant commented Jul 16, 2021

ry Jul 19, 2021

Choose a reason for hiding this comment

pseudosavant Jul 20, 2021

Choose a reason for hiding this comment

pseudosavant Jul 20, 2021

Choose a reason for hiding this comment

pseudosavant Jul 21, 2021

Choose a reason for hiding this comment

ry Jul 21, 2021

Choose a reason for hiding this comment

pseudosavant Jul 21, 2021

Choose a reason for hiding this comment

kt3k commented Jul 20, 2021

pseudosavant commented Jul 21, 2021

kt3k commented Jul 22, 2021

kt3k left a comment

Choose a reason for hiding this comment

pseudosavant commented Jul 23, 2021

Add range request and etag support to `file_server.ts` #1028

Add range request and etag support to `file_server.ts` #1028

CLAassistant commented Jul 12, 2021 •

edited

Loading

ry left a comment •

edited

Loading