-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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 tests to verify preload caching behavior #31539
Conversation
Submitted implementation bugs: |
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.
Looks good! A couple of questions
<script src="/preload/resources/preload_helper.js"></script> | ||
<script> | ||
|
||
const crossOriginHost = 'https://{{host}}:{{ports[https][0]}}' |
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.
We typically use get-host-info.sub.js for those things, as it encapsulates the "sub" parts and is generally more readable.
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.
Yea, originally it needed to be as a sub.html, I'll revise
for (const resourceConfigName of configNames) { | ||
for (const preloadConfigName of Object.keys(configs)) { | ||
if (resourceConfigName === 'same-origin' && preloadConfigName !== 'same-origin') | ||
continue; |
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.
Nit: ||
those 2 conditions? (instead of having those separate if
statements)
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.
Also, we're skipping those cases because in those cases we'd get a different URL?
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.
Because if it's the same origin the CORS attributes are ignored anyway. Will try to clarify with a comment
}, `Loading ${type} (${resourceConfig}) with link (${preloadConfig}) should ${expected} the preloaded response`); | ||
} | ||
|
||
for (const resourceTypeName in resourceTypes) { |
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.
s/in/of/ ?
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.
No, it's a key iteration
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.
Fixed to use Object.entries instead
} | ||
|
||
const configs = { | ||
'same-origin': {crossOrigin: false, attributes: {}}, |
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.
Can you add a comment saying what each of the config values does?
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.
Done
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.
Fixed CR comments.
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 % nits
<script src="/common/get-host-info.sub.js"></script> | ||
<script> | ||
|
||
const {REMOTE_HOST, HTTPS_PORT} = get_host_info(); |
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.
Nit: you can use HTTPS_REMOTE_ORIGIN
instead of those two
|
||
} | ||
|
||
for (const resourceTypeName in resourceTypes) { |
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.
Leftovers?
Check that preloaded resources are reused/ignored based on the parameters available to a preload link (href, crossorigin, as), and do not rely on other parameters (e.g. whether a script is a module). See whatwg/html#7260 and whatwg/fetch#1342
Before any particular fetch steps are performed, see if there is a matching request already in the preload store and consume it. This is called from the main fetch to avoid race conditions. Depends on whatwg/html#7260, and together they fix #590. Tests: web-platform-tests/wpt#31539.
Before any particular fetch steps are performed, see if there is a matching request already in the preload store and consume it. This is called from the main fetch to avoid race conditions. Depends on whatwg/html#7260, and together they fix whatwg#590. Tests: web-platform-tests/wpt#31539.
Check that preloaded resources are reused/ignored based on the
parameters available to a preload link (href, crossorigin, as),
and do not rely on other parameters (e.g. whether a script is a module, or an image is decoded successfully).
See whatwg/html#7260
and whatwg/fetch#1342