-
Notifications
You must be signed in to change notification settings - Fork 75
feat: add assetsHash for cache naming #86
base: master
Are you sure you want to change the base?
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.
Thanks for this PR, it's actually something that I'll be needing myself in the near future.
Just got a few tiny remarks about the tests, but otherwise I'm quite happy.
src/index.spec.js
Outdated
const transformOptions = serviceWorkerOption => { | ||
expect(serviceWorkerOption) | ||
.to.have.property('assetsHash') | ||
.that.is.a('string') |
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.
As the input is known, so is the output - could you update the test case and let it test against the actual expected value of the hash?
options: fakeOptions, | ||
assets: { | ||
[filename]: { | ||
source: () => '', |
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.
Perhaps you can add a second test case, with a different mocked source, as evidence that the hash is something else when the contents change.
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.
Oh I think we've a design decision here. Maybe I didn't explain fully, but currently only the filenames get hashed not the contents. In my project I use filenames with hashes so there is no problem with this. On the other hand after reading this I think the only real use case to make the assetsHash include the content would be to make the serviceworker byte different when the assets change, but not their filenames. In this case it would probably be easier to just make the jsonStats
include the compilation hash by changing this line to true.
What do you think @woutervanvliet ?
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.
Think I indeed misunderstood the point of your PR, but yes - my approach would be to have some kind of value, that's the same between builds of the same code, but different when sourcecode changes.
Tbh; I'm not that familiar with webpack internals, but it sounds like enabling the hash would solve the same problem as your PR does.
@devCrossNet @oliviertassinari What do you say?
pls merge this uwu |
What is accomplished by your PR?
This PR adds another parameter to the options passed to the
transformOptions
function. It's an hash of theassets
-filename array. I use this for the naming of the cache in the SW, because thejsonStats
param does not contain anything useful for naming. Whenever the array of file(name)s to cache changes, so does theassetsHash
therefore a new cache is opened and loaded with the new files.Example implementation (a
transformOptions
function which passes on theassetsHash
is needed):Is there something controversial in your PR?
Maybe the default
transformOptions
function should pass on theassetsHash
?Checklist