-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 header to IA CDN endpoint #8944
Conversation
Can you please explain why you chose defer vs async for each of the scripts? We should investigate which is best. @QuantuM410 |
@RayBB From what I understood using async and defer does the same job just async doesnt have an order of execution as well as it executes stuff in the background and then doesnt wait while defer wait for the loading of DOM. Async would have been fine if both the files work independently I think and I was not really sure if thats the case hence I used |
@QuantuM410 that makes sense, good explanation. Can you do a comparison of the page load time on regular internet and "slow 3g" (or similar in your dev tools) of the before and after? So screenshots of dev tools loading times of:
For each request we should already have the loaded the page once before so the scripts are cached. You might need to clear your cache between each screenshot so it's not accidentally left in cache for the "before" tests. Thanks! |
@@ -38,7 +38,7 @@ | |||
</script> | |||
|
|||
<!-- Must be loaded after jquery --> | |||
<script src="/cdn/archive.org/analytics.js" type="text/javascript"></script> | |||
<script src="/cdn/archive.org/analytics.js" type="text/javascript" defer></script> |
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.
I don't think we can make this one defer
; if you'll notice below we have a raw <script>
tag which makes use of window.archive_analytics
(which is exposed by this script). If we make this defer, I think this script will run after the script below.
Hey @RayBB ! I did a page load time comparison and I feel adding defer didn't do much and also @cdrini quotes about removing defer from the analytics script as its being further used in a script, moreover I felt this needed a bit more discussion hence I have removed the tags as for now as I am a bit confused :) Here are the results PS: I tested it after removing defer from analytics.js script. I didn't really find any significant change after observing it |
@QuantuM410 @cdrini I did some testing of my own and here are the results:
Overall, we are seeing a very nice improvement in speed when on slow connections :) @QuantuM410 I think your results are different because the cache is disabled in the devtools. @cdrini how long do you think the cache time should be? I think one day is very safe but I'm really not sure how likely those libraries are to change. @QuantuM410 I'll guess that we should only cache for one day in case those libraries do change. So maybe make that change and if Drini agrees he'll merge it and if not he'll ask for a different duration. |
Ah I though I needed to test the page load storing no cache for each cdn request to test the feasibility of |
Closes #8927
This PR adds a cache header to IA CDN requests and provides a defer script tag to the respective scripts
Technical
Testing
Screenshot
Stakeholders
@RayBB