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

Feature/website cache busting #813

Merged
merged 7 commits into from
Aug 23, 2021
Merged

Feature/website cache busting #813

merged 7 commits into from
Aug 23, 2021

Conversation

MichielPater
Copy link
Contributor

Added cache busting, so that cache will be cleared on client side whenever devicon.json or SVG files are updated.

Thomas-Boi
Thomas-Boi previously approved these changes Aug 16, 2021
Copy link
Member

@Thomas-Boi Thomas-Boi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Michiel and I have discussed this with each other and the changes look good. I also tested the URL to fetch devicon.json manually and the result is still the same.

After all, the ? adds query params but since the API don't accept params for this, nothing happens.

Panquesito7
Panquesito7 previously approved these changes Aug 16, 2021
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, it looks good. I'll better let @amacado check if this PR's fine, though. Thank you for your contribution! 😄👍

docs/index.html Outdated
@@ -135,7 +135,7 @@ <h3 class="cbp-ig-title">{{icon.name}}</h3>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/1.2.19/angular.min.js"></script>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/1.2.19/angular-sanitize.min.js"></script>
<script src="//ajax.googleapis.com/ajax/libs/angularjs/1.2.19/angular-animate.min.js"></script>
<script src="assets/js/script.js"></script>
<script src="assets/js/script.js?2.13.0"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a static value of ?2.13.0 will probably not achieve the required result, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only for the script file, to make it update with the cache busting feature. From this point, the script only needs to be cache busted whenever that file changes, but wouldn't as long as it stays the same. The cache busting is done within the file.

Copy link
Member

@Thomas-Boi Thomas-Boi Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file is in angular (which it is), we can also just use variables defined in $scope. That way we never have update the version manually whenever the script.js changes. While we don't do it now, perhaps later down the road we'll need to update the website.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. However, I don't think we can use the variables we created within the script file itself. But we could use others, or move the definition outside of the file.

Or maybe there is even caching set outside of these files, like in a configuration or the HTML file. The PR was intended to be a small change though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can embed angular code in? Since angular files were listed before the script.js, perhaps we can do something like:

<script src='script.js?{{ (new Date()).getTime() }}' />

I'm not 100% sure this is feasible with angular but I'd think it's possible. As for caching setting for the file itself, we'll have to check the GitHub pages doc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could replace this <script> tag with the following?

<script>
	document.write("<script src='assets/js/script.js?" + Date.now() + "'><\/script>");
</script>

@Thomas-Boi
Copy link
Member

Thomas-Boi commented Aug 23, 2021

Just tested @MichielPater's solution for cache-busting JS and it works well
image

The only thing that's needed is putting the script tag for the document.write in its separate `<script>. If you can add the changes as you suggested, I think we can merge the PR

Panquesito7
Panquesito7 previously approved these changes Aug 23, 2021
Panquesito7
Panquesito7 previously approved these changes Aug 23, 2021
Thomas-Boi
Thomas-Boi previously approved these changes Aug 23, 2021
Copy link
Member

@Thomas-Boi Thomas-Boi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! Thanks for the help 😄 @MichielPater

@Thomas-Boi Thomas-Boi changed the base branch from master to develop August 23, 2021 19:10
@Thomas-Boi Thomas-Boi dismissed stale reviews from Panquesito7 and themself August 23, 2021 19:10

The base branch was changed.

@Thomas-Boi Thomas-Boi merged commit 92417d4 into devicons:develop Aug 23, 2021
This was referenced Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants