kornelski / http-cache-semantics

RFC 7234 in JavaScript. Parses HTTP headers to correctly compute cacheability of responses, even in complex cases
http://httpwg.org/specs/rfc7234.html
BSD 2-Clause "Simplified" License
244 stars 27 forks source link

Dropping trustServerDate is a breaking change #32

Closed isaacs closed 3 years ago

isaacs commented 3 years ago

Hey, I completely agree with the justification provided in the README, but dropping support for this option is a breaking change, in the SemVer sense of "a change that requires downstream users to alter their code in order to continue working properly with the library".

This broke a few of my tests, and I now have to go and figure out whether I want to pin the version to 4.0, alter the tests to provide an accurate date header, or just live with the red CI. My case is not so horrible, since it does work fine except in the synthetic edge cases that the test is (no longer) exercising, but it's entirely possible that someone using this library had production code broken as a result.

I recommend reverting this change in a 4.1.1 release, and then re-publishing it in a 5.0.0 release. There are two reasonable ways to do this:

legacy branch

This is the approach I typically use when I am on the other end of an issue like this.

git checkout -b v4-legacy
git reset --hard eb7028f13f1b2e6978834c9a51b9d3f0d733ab77
# add `"publishConfig": { "tag": "legacy" }` to package.json
git commit -m "publish to 'legacy' tag on npm"
npm version 4.1.1
npm publish
git checkout master
npm version major # bumps to 5.0.0
npm publish

Thereafter, any patches you want to land on the v4 branch (where server date is trusted) you can do on the v4-legacy branch, and npm publish, without worrying that it clobbers the "real" latest release on npm.

revert and re-revert

This is a simpler single-branch approach if you're pretty sure you're not going to ever have to touch v4 again.

git revert ed83aec75be817967cdac2663907d060fdc6adc3
git revert 1b359803db768e0b3f5f8051aa3ea50e27c8d096
git revert 2c2fac2d9763137795cac524db2593de9cba70ac
npm version 4.1.1
npm publish
# re-land the commits that you just reverted
# note that the last one will have a conflict on the package.json version,
# which will have to be resolved by setting it back to 4.1.1 instead of 4.1.0
git cherry-pick 2c2fac2d9763137795cac524db2593de9cba70ac
git cherry-pick 1b359803db768e0b3f5f8051aa3ea50e27c8d096
git cherry-pick ed83aec75be817967cdac2663907d060fdc6adc3
npm version 5.0.0

And never look back.

kornelski commented 3 years ago

When I was refactoring this code (to make a Rust version) I've realized that this feature was not working well, was making age computation complex and inaccurate, and was against the RFC. For a simple package that claims to implement the RFC, a huge RFC-breaking fudge was a bad idea, so it had to go.

Sorry for the churn. I should have bumped major version then. However, now that the damage has been done, reverting the change and creating another major version would only create even more failed tests and upgrade churn for users. So I'm going to keep it as-is.

isaacs commented 3 years ago

I understand. Tough call, but you're right, the moment has passed, I suppose. Thanks!