-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add Cache-Control
defaults to all the demo store routes
#599
Conversation
export const CACHE_SHORT = generateCacheControlHeader(CacheShort()); | ||
export const CACHE_LONG = generateCacheControlHeader(CacheLong()); | ||
export const CACHE_NONE = generateCacheControlHeader(CacheNone()); |
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.
What do you think about doing this instead?
export const CACHE_SHORT = generateCacheControlHeader(CacheShort()); | |
export const CACHE_LONG = generateCacheControlHeader(CacheLong()); | |
export const CACHE_NONE = generateCacheControlHeader(CacheNone()); | |
export const HEADERS_CACHE_SHORT = Object.freeze({'Cache-Control': generateCacheControlHeader(CacheShort())}); | |
export const HEADERS_CACHE_LONG = Object.freeze({'Cache-Control': generateCacheControlHeader(CacheLong())}); | |
export const HEADERS_CACHE_NONE = Object.freeze({'Cache-Control': generateCacheControlHeader(CacheNone())}); |
Then, in loaders we can just use {headers: HEADERS_CACHE_SHORT}
instead of typing "cache-control" everywhere.
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.
Hmm, not a bad idea, though it makes it a bit more complex to extend and define a custom header
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.
mmm .. I like what Bret have:
If we go for Fran's suggestion ...
// it's easy when nothing else is needed
json({}, {headers: HEADERS_CACHE_SHORT})
// it's weird when you need to add something else
json({}, {
headers: {
...HEADERS_CACHE_SHORT,
'some-header': 'value'
}
})
whereas Bret's suggestion would be unambiguous
json({}, {
headers: {
'Cache-Control': HEADERS_CACHE_SHORT,
'some-header': 'value'
}
})
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.
How common is it to add extra headers, though? Wouldn't most of the times just be headers: HEADERS_CACHE_SHORT
?
Plus, even in that situation I'd personally still prefer writing ...
than 'Cache-Control'
since for some reason I always misspell header names 😂
But just a random idea though, the current version is good as well 👍
export const CACHE_SHORT = generateCacheControlHeader(CacheShort()); | ||
export const CACHE_LONG = generateCacheControlHeader(CacheLong()); | ||
export const CACHE_NONE = generateCacheControlHeader(CacheNone()); |
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.
mmm .. I like what Bret have:
If we go for Fran's suggestion ...
// it's easy when nothing else is needed
json({}, {headers: HEADERS_CACHE_SHORT})
// it's weird when you need to add something else
json({}, {
headers: {
...HEADERS_CACHE_SHORT,
'some-header': 'value'
}
})
whereas Bret's suggestion would be unambiguous
json({}, {
headers: {
'Cache-Control': HEADERS_CACHE_SHORT,
'some-header': 'value'
}
})
WHY are these changes introduced?
Cache-Control
defaults to all the demo store routesWHAT is this pull request doing?
HOW to test your changes?
Post-merge steps
Checklist