Skip to content
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 busting variable to Pi-hole js/css #1550

Merged
merged 6 commits into from
Sep 14, 2020
Merged

Add cache busting variable to Pi-hole js/css #1550

merged 6 commits into from
Sep 14, 2020

Conversation

tjeffree
Copy link
Contributor

@tjeffree tjeffree commented Aug 11, 2020

Add cache busting variable to Pi-hole js/css

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

After every Pi-hole update there are a raft of messages from users who have broken dashboards due to old Javascript files being cached by browsers. This PR fixes the problem hopefully saving a lot of support work.

How does this PR accomplish the above?:

This PR adds a simple variable to the dashboard Javascript files based on a file creation time that will update along with the Pi-hole update. It also adds the same variable to Pi-hole css files in the header.

What documentation changes (if any) are needed to support this PR?:

None required.

@PromoFaux
Copy link
Member

This was supposed to have been sorted with some changes to the lighttpd.conf file in core, however it doesn't appear to have worked. And in any case, it would be nice to have this independent of the actual webserver.

Is it worth, in your opinion, adding this same variable to all other calls to js/css files?

@tjeffree
Copy link
Contributor Author

Certainly could, I only focused on the ones that appear to be causing the most issues with updates.

There could be a risk of falling in to a trap of where to stop - images for instance could even be included.

Maybe for this PR I could include all CSS and JS in header.php?

@PromoFaux
Copy link
Member

Yeah, I see what you mean.

Any one of the scripts could change between releases (vendor scripts less likely, but still possible), but for instance this one was changed.

@PromoFaux PromoFaux requested a review from XhmikosR August 11, 2020 10:35
@tjeffree
Copy link
Contributor Author

Maybe any that match pi-hole/js/[a-zA-Z0-9_\-]+\.js would be the best way to go. I'll just need to check that any page that includes one of these also requires header.php - looks like about 20 files so not too bad.

@XhmikosR
Copy link
Contributor

I don't see why it didn't work... cache-control: max-age=0 is present and it should work.

Regardless, cache busting is a good idea. That way we can increase max-age.

  1. Whatever solution we go with, it should be applied to all static files, at least, all CSS and JS files
  2. Not sure about any performance issues, my guess is there won't be any but it'd be nice to verify
  3. JS should not be in header. In fact I have a PR to move everything in footer. We don't need JS to be render blocking.

@tjeffree
Copy link
Contributor Author

  1. Whatever solution we go with, it should be applied to all static files, at least, all CSS and JS files
  2. Not sure about any performance issues, my guess is there won't be any but it'd be nice to verify
  3. JS should not be in header. In fact I have a PR to move everything in footer. We don't need JS to be render blocking.
  1. I've applied it to all CSS and JS in header.php along with all JS in scripts/pi-hole/js
  2. I use this technique in production environments on large scale projects, usually using a version number that's incremented with deployment. Until the cacheVer is changed it'll be as performant as the variable not being there at all
  3. Agreed. Although on local network installs this probably is not much a performance hit.

(p.s. sorry about pull request mess, not used to using sign off)

@PromoFaux
Copy link
Member

@tjeffree please could you rebase on devel? We forgot to update devel from master after we did the last release, which is done now! Sorry. And don't worry about the commit mess, you should see some of my PRs.... 🙄

@tjeffree
Copy link
Contributor Author

@PromoFaux Ok, I think that's done!

@PromoFaux
Copy link
Member

Thanks, will try to find the time to fully review later

db_graph.php Outdated Show resolved Hide resolved
db_lists.php Outdated Show resolved Hide resolved
db_queries.php Outdated Show resolved Hide resolved
groups-adlists.php Outdated Show resolved Hide resolved
groups.php Outdated Show resolved Hide resolved
@tjeffree
Copy link
Contributor Author

I wasn't planning on adding vendor scripts as part of the cache busting. I'm not sure how vendor files are added to the project, would it be likely that they are replaced with the same names? I've always found it useful to add third party library files with version numbers included in the file name.

I did miss one though, must have been after rebasing.

@PromoFaux
Copy link
Member

It was only because I noticed you did the vendor links in the header, is all.

Whilst we do have an npm config, that's only for running the linters etc. Everything else in the vendor directory has been added manually

@tjeffree
Copy link
Contributor Author

I see, prefer if I just add it to all then?

@PromoFaux
Copy link
Member

PromoFaux commented Aug 21, 2020

Yeah, might as well! They're less likely to change, but they do change from time to time

@tjeffree
Copy link
Contributor Author

All done!

@tjeffree
Copy link
Contributor Author

This is back up to date with the latest devel again.

@PromoFaux PromoFaux merged commit 5c8cba3 into pi-hole:devel Sep 14, 2020
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/issue-dashboards-keep-spinning-pi-hole-docker-synology-latest-version-5-1-2/38372/4

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-core-web-v5-2-and-ftl-v5-3-released/40909/1

@yubiuser yubiuser mentioned this pull request Jan 13, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants