-
Notifications
You must be signed in to change notification settings - Fork 11
Allow setting cache control headers - alternative #228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for configurable cache control headers as a non-breaking alternative to PR #227. The implementation prioritizes cache-control headers defined via response header wildcards over appConfig values, changes the default s-maxage from 0 to 60, and only adds cache headers when explicitly configured.
Key Changes:
- Modified cache control logic to support override via
adp-cache-controlresponse headers - Changed default
s-maxagevalue from 0 to 60 seconds - Updated to return
nullinstead of default cache control strings when values are not explicitly configured - Upgraded Parcel from ^2.7.0 to ^2.15.4
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| package.json | Upgrades Parcel dependency from version 2.7.0 to 2.15.4 |
| lib/remote-storage.js | Implements cache control header override logic, modifies default behavior to only set cache headers when explicitly configured, adds JSDoc for canAddHeader method, and changes Metadata to always be set as an object |
Comments suppressed due to low confidence (5)
lib/remote-storage.js:145
- Missing space after
ifkeyword. The codebase consistently usesif (with a space (see other occurrences in the file). Please add a space betweenifand the opening parenthesis for consistency.
*/
lib/remote-storage.js:301
- Potential bug: If
appConfig.htmlCacheDurationis undefined, this will produce an invalid cache-control string likes-maxage=60, max-age=undefined. Consider checking if the cache duration property exists before constructing the string, or returningnullif it doesn't exist (similar to theelsecase on line 309).
fileBatch = files.splice(0, batchSize)
lib/remote-storage.js:303
- Potential bug: If
appConfig.jsCacheDurationis undefined, this will produce an invalid cache-control string likes-maxage=60, max-age=undefined. Consider checking if the cache duration property exists before constructing the string, or returningnullif it doesn't exist (similar to theelsecase on line 309).
return allResults
lib/remote-storage.js:305
- Potential bug: If
appConfig.cssCacheDurationis undefined, this will produce an invalid cache-control string likes-maxage=60, max-age=undefined. Consider checking if the cache duration property exists before constructing the string, or returningnullif it doesn't exist (similar to theelsecase on line 309).
lib/remote-storage.js:307 - Potential bug: If
appConfig.imageCacheDurationis undefined, this will produce an invalid cache-control string likes-maxage=60, max-age=undefined. Consider checking if the cache duration property exists before constructing the string, or returningnullif it doesn't exist (similar to theelsecase on line 309).
* Get cache control string based on mime type and config
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
This is an alternative to #227 and is non-breaking.
Priority is given to cache-control headers defined with normal custom-header wildcards.
If appConfig values are present for htmlCacheDuration(|js|css|image) they will be used, but s-max-age will be set to 60 by default.
If no cache headers are explicitly set, they will not be added to the metadata.
note: there will be another pr to app-config to remove defaults and only load values if explicitly specified
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: