fix: send the immutable Cache-Control directive as its own field-line - #315
Open
bilashcse wants to merge 3 commits into
Open
fix: send the immutable Cache-Control directive as its own field-line#315bilashcse wants to merge 3 commits into
bilashcse wants to merge 3 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
sendappends theimmutabledirective to themax-agevalue, so a single field-line goes on the wire:public, max-age=<seconds>on its own is a value an HTTP/3 QPACK encoder can match against the static table, but the combined value is not, so turning onimmutablemakes the header more expensive to encode. This was reported in expressjs/serve-static#149, where the suggestion (and the MDN note quoted there) is to split the value across two field-lines instead.Fix
When
immutableis enabled,Cache-Controlis now set as two field-lines:HTTP caches combine repeated
Cache-Controlfield-lines, so the effective set of directives is unchanged and clients keep seeing the same behaviour. Nothing changes whenimmutableis disabled.Tests
Two cases were added under
describe('immutable')that inspectrawHeadersto assert the exact field-lines that go on the wire, one withimmutable: trueand one withimmutable: false, together with ashouldHaveHeaderValueshelper. The existingshould set immutable directive in Cache-Controltest is left untouched and still passes, because Node joins repeated field-lines when populatingres.headers.The suite was run on a fork before opening this PR: lint plus Node.js 18-25 on both ubuntu-latest and windows-latest all pass. The only failing job there is
coverage, which cannot reach Coveralls from a fork.Docs
README.mdnow documents the two field-lines under theimmutableoption and explains why the value is split.Refs expressjs/serve-static#149