-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Conversation
Alex Recommends ReportAlex recommends the following language changes, but Alex is a regular expression based algorithm, so take them with a grain of salt. ✨ 🚀 ✨ Nothing to Report ✨ 🚀 ✨ |
Deploy preview for web-dev-staging ready! Built with commit a3a0b89 |
aa22019
to
5146585
Compare
Please re-assign to me when it's ready for review. |
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.
Overall this looks really good, left some notes / suggestions for consideration.
|
||
* **Dynamic content** | ||
|
||
Dynamic content is content that changes frequently. An API response and a store homepage are examples of this content type. However, the fact that this content changes frequently doesn't necessarily preclude it from being cached. During periods of heavy traffic, caching these responses for very short periods of time (e.g. 5 seconds) can significantly reduce the load on the origin server, while having minimal impact on data freshness. |
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.
Agreed - controllable/on-demand purging allows more aggressive caching and is a major CDN benefit/feature as well as other invalidation mechanisms.
@khempenius looks like this is waiting on you to review last round of feedback |
@dotjs @mnot-fastly thank you for the feedback! |
@kaycebasques ready for your review |
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.
Looking good, lots of in-depth ideas that I've never seen before
Tip: Use the batch commit suggestions workflow to incorporate my requested changes faster.
Please break all lines at 100 characters. If you're using VS Code, Rewrap makes it pretty easy to do. The Docs To X Google Docs Add-On also supports this. One reason is because GitHub's UI isn't good at formatting requested changes when lines are long:
I noticed a few instances of the passive voice and may want to read again and see if I missed any
We should link to this from web.dev/fast, yeah?
(The requested changes below were auto-generated by the web.dev content review extension)
- Resize the hero image: https://web.dev/handbook/markup-media/#hero
- Resize the width of the following images to 1600px or less:
- cdn2.png
- Add a subheading to the page: https://web.dev/handbook/yaml-front-matter/#subhead
|
||
### HTTP/2 & HTTP/3 | ||
|
||
HTTP/2 and HTTP/3 both provide performance benefits over HTTP/1. Of the two, HTTP/3 offers greater _potential_ performance benefits. HTTP/3 isn't fully standardized yet, but it will be widely [supported](https://caniuse.com/#feat=http3) once this occurs. |
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.
Is there a reason that we are making this strong claim about HTTP/3? It seems risky to say "it will be supported once it gets fully standardized" especially since there are factors outside of our control (other browser vendors)
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 should be OK. H3 is in all major browsers at the moment - albeit behind a flag. My original wording was less strong than this but it was suggested that this would be more accurate.
Co-authored-by: Kayce Basques <kayce@google.com>
Co-authored-by: Kayce Basques <kayce@google.com>
2bfa496
to
2ed2bf3
Compare
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.
LGTM. I'll merge this Tuesday 9/21.
Fixes #2416