Fix bug in handling of invalid if-modified-since/last-modified values

[benurb]

Hi @dougwilson

thanks for this very useful lib, but unfortunately I found two little bugs.

1. The tests for invalid values in if-modified-since/last-modified were broken and therefore covering up ...
2. a actual bug in the handling of invalid values in if-modified-since headers.
   I split up the changes in two commits so it's easier for you the reproduce the behavior. With only the test changes applied you can see that one test is actually failing and with both commits applied all tests should be green again 👍

Best regards,
Ben

[dougwilson]

Wow, thanks! What a goof :) The PR should just be tweaked a bit to handle the fact that a very popular library, datejs, overwrites Date.parse and actually returns just a falsy-value instead of NaN on a date parse failure. This causes isNaN to have the opposite value of what it should, which could lead to 304 responses when that was not appropriate.

I'm only aware of this because a while back a release of a related library started using isNaN checks on the result of Date.parse and it turns into a Whole Thing(TM) and so I modified many libraries, like this one, to still function correctly as long as the result of Date.parse was any falsy value.

This new logic will cause those users to start getting unconditional 304 responses in normal usage. For example if a the last modified and modified since are both empty, you would expect this to be isNaN(NaN) || isNaN(NaN) || NaN > NaN which results in true, but under datejs, that line is actually isNaN(null) || isNaN(null) || null > null which results in false

:(

[benurb]

Oh, ok. Didn't know about the Date.parse problem. What solution do you prefer?
One "long" short circuit clause?

var lastModifiedTimestamp = Date.parse(resHeaders['last-modified'])
var modifiedSinceTimestamp = Date.parse(modifiedSince)
var modifiedStale = !lastModifiedTimestamp || isNaN(lastModifiedTimestamp) ||
  !modifiedSinceTimestamp || isNaN(modifiedSinceTimestamp) ||
  !lastModifiedTimestamp > modifiedSinceTimestamp

Or two "short" short circuits?

var lastModifiedTimestamp = Date.parse(resHeaders['last-modified']) || NaN
var modifiedSinceTimestamp = Date.parse(modifiedSince) || NaN
var modifiedStale = isNaN(lastModifiedTimestamp) || isNaN(modifiedSinceTimestamp) ||
  lastModifiedTimestamp > modifiedSinceTimestamp

[dougwilson]

Neither of them seem to work correctly for the one date / time in which Date.parse returns 0 (unix epoch).

[benurb]

Hey @dougwilson,
yep, you're right. I've change the code a bit and added tests for all the edge cases you mentioned.

[dougwilson]

I'm not sure if this makes sense, because you can use earlier dates / times, so not sure why this is the "earliest possible".

[benurb]

Yeah you're right. I'll change that to something like 'with unix epoch in If-Modified-Since', ok?

[dougwilson]

If this is going to actually have a test, just test with what we really care about specifically: the datejs patching.

[benurb]

Mhh ok. I haven't had a look at datejs yet, but do you suggest adding it as a devDependency or rather make the mock more similar to the datejs patching? Or are these tests a bad idea in general?

[dougwilson]

A devDep is just fine, as they are available during the testing. If you want to just not have any tests, that's OK too, but if you wanted to make some patching tests, I'd say to either (a) directly use datejs in them or (b) make the mocking patches actually function like the datejs does (which afaik these are different from what datejs returns).

[benurb]

I was not able to use datejs without polluting global objects, but I updated the mock to return the same result as datejs for the strings in the tests. If you don't like this I would rather remove the tests again.

[benurb]

Hi @dougwilson
I addressed your comments. Let me know what you think 👍

[benurb]

Hey @dougwilson
any additional thoughs on this PR?

[dougwilson]

Hi @benurb I'm really sorry I let this fall though, truly sorry. I just got it merged in now.