Skip to content
This repository was archived by the owner on Aug 10, 2023. It is now read-only.

add optional feature to check modified file modified time to return 304#23

Closed
avimar wants to merge 3 commits into
restify:masterfrom
avimar:master
Closed

add optional feature to check modified file modified time to return 304#23
avimar wants to merge 3 commits into
restify:masterfrom
avimar:master

Conversation

@avimar

@avimar avimar commented Apr 11, 2016

Copy link
Copy Markdown

restify/node-restify#994

#17
@Dzitu what version of windows return milliseconds? My Windows 7 always ends with 000 meaning it's only accurate to seconds.

@avimar

avimar commented Apr 11, 2016

Copy link
Copy Markdown
Author

(Just to note, this effectively ignores bad if-modified-since headers, it won't trigger an error.)

avimar added a commit to avimar/node-restify that referenced this pull request Apr 11, 2016
@Dzitu

Dzitu commented Apr 11, 2016

Copy link
Copy Markdown

@avimar Windows 7 here as well, following script:
var fs = require("fs");
var path = "static.js";
fs.stat(path, function(err, stats) {
if(!err)
console.log(new Date(stats.mtime).getMilliseconds());
});

Returns 491 for me.

@avimar

avimar commented Apr 11, 2016

Copy link
Copy Markdown
Author

@Dzitu Interesting!
Turns out it not windows or linux, but rather... Date.parse() strips milliseconds.

I don't know if that will ever change..
but since conditional and my implementation use Date.parse() it all seems OK.

var fs=require('fs');
var stat = fs.statSync('myFile');
console.log(new Date(stat.mtime).getMilliseconds());
//769
console.log(new Date(stat.mtime).valueOf())
//1449737820769
console.log(Date.parse(stat.mtime))
//1449737820000 <-- no milliseconds

@DonutEspresso

Copy link
Copy Markdown
Member

LGTM, @micahr or @yunong to verify please.

Comment thread lib/plugins/static.js
if (opts.charSet) {
var type = res.getHeader('Content-Type') +
'; charset=' + opts.charSet;
res.setHeader('Content-Type', type);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this override the res.set('Content-Type') from before? If so, perhaps they should be using the same method?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't know: I only moved that code, I didn't write it. It looks like it appends it to the current content-type.

@micahr

micahr commented May 4, 2016

Copy link
Copy Markdown
Contributor

I added a few comments. I'd love to see a test or two for this change as well.

@avimar

avimar commented Jun 23, 2016

Copy link
Copy Markdown
Author

@micahr is tests holding this up?
I looked at the test suite and I don't really understand it. I'd assume the same test that applies to the if-conditions can be re-applied with a temp file and running static in the middle.

@avimar

avimar commented Oct 25, 2016

Copy link
Copy Markdown
Author

@micahr - I addressed your comments. What now?

@yunong

yunong commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

@avimar the only thing left now is to add some unit tests for the new behaviour.

@retrohacker

Copy link
Copy Markdown

Hey @avimar,

It looks like @yunong requested tests a while back, though you were caught up on the testing framework and requested assistance. We are in the process of moving the plugins back into the main repo. If this work is still important to you, please re-open a PR over on that repo and I would be more than guide you through putting together some tests ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants