-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-manifest): add cache busting to icon url #8343
Conversation
Instead of a random id which will change on every build. How about instead we use an id derived from the content digest of the icon file which will stay consistent across builds. |
Just 2 things:
|
ac23a2b
to
b29b46e
Compare
Let me know if this works. |
@github0013 looks like the linter is failing! Easiest way to get these passing is to run |
921f6b9
to
f3e977a
Compare
I'll take a look at this first thing tomorrow; sorry for the delay! |
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.
This looks good. Left two comments/questions!
@github0013 any chance you can pursuit this one? 👍 it's a great feature to have. |
@pieh @jlengstorf @DSchau @github0013 I'd like to get this merged, i and others seem interested in solving this problem. I'm currently seeing a couple outstanding issues on this PR.
Tests and linting I can handle. I see there is discussion above concerning making Finally, how do we solve adding the digest to all image references? My inclination is to modify the name of the actual files Thanks folks! |
@moonmeister thanks, feel free to fix the tests if you like. (unsure if you can edit this PR 😛). Cachebusting is fine with querystrings. There is no real benefit about any of them. The downside to querystring busting is that some CDN providers give you the option to omit the querystring as part of the cachekey. Perhaps it's a good idea to do add an option to create a filename 🙊 . I can take care of the utils part |
@wardpeet I was able to pull the branch just fine, haven't tried pushing yet but guessing it'll work. Yeah, this probably needs to be behind a flag anyway (even if it's on by default), I can have options for [none, query, name] and the code changes how it does cache busting accordingly. This shouldn't be too hard and I have time. If you get the utils done before me then awesome, otherwise I'll just use an internal util in commons and this can be migrated later once the Gatsby util is available. |
66cbf31
to
7b9d977
Compare
Looks like I have some rebasing and might need to revert my move to async/await to be node 6 compatible. And then I need to write tests. |
@moonmeister async/await is fine - we run babel on source files when packaging for release so resulting code will be node6 compatible |
7b9d977
to
da0b49d
Compare
@pieh All ready for review and merge! |
yarn lint:js --fix
used internal digest generator till the global one is available converted the node api to use ASYNC await to simplify new code added cache busting to favicon, manifest, and legacy head links
e31f906
to
b62da7a
Compare
…re suppose to be.
Should be good now @pieh |
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.
Look good to me, I fixed merge conflicts and cleaned it up a little bit.
Thank you for your patience! 🙏
Holy buckets, @github0013 — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
Browsers cache favicons, and it's hard to update them without a random parameter in the favicon link.