-
Notifications
You must be signed in to change notification settings - Fork 337
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 the Priority Hints changes to the fetch spec #1523
Conversation
From what I understand, Mozilla will be implementing it so I'll ping them to commit to the implementation on the issue. |
That is correct. We are tracking this in https://bugzilla.mozilla.org/show_bug.cgi?id=1797715 |
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.
Thank you for working on this!
I'm not seeing the outcome of WICG/priority-hints#69 incorporated here.
We might also want to say a bit more about "internal priority".
Please also draft a commit message (as a comment) that links the appropriate HTML changes and makes it clear that all of this essentially obsoletes the Priority Hints specification. (I'm assuming that's the intent given there's no normative references.)
@annevk I think I got most of the changes done and the last commit message includes the obsoleting language. I added a comment to the render-blocking issue to discuss it a bit more before we figure out what the changes mean for the spec (needless to say, this merge will wait on the outcome of that thread). |
I removed I also have |
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.
What will happen to the Priority Hints specification? Does it essentially disappear?
As far as the existing Priority Hints spec goes, once both HTML and fetch are updated I'm planning on removing most of the content but leaving a placeholder with some high-level details and links to the relevant parts in the HTML and fetch specs. That way any external links in blog posts and articles will still find their way to the specs with the relevant details. |
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.
The Request constructor is not correct:
- It copies priority rather than internal priority.
- When priority is given this is not taken into account. I suspect that when it's given we want to set request's priority to it and we want to set internal priority to null (so request's priority will be used upon fetching to initialize it). Or would we want to adjust internal priority instead in some manner so the existing prioritization information is taken into account?
Thanks, looks like I accidentally deleted I also added a step 27 to the constructor to take init's |
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.
Thanks! This looks good to me. I have a couple final nits I can take care of when merging.
Where is the corresponding HTML PR again?
Could you post a suggested commit message in a new comment that includes all the relevant details and pointers to issues/documents/PRs? You can use https://github.com/whatwg/meta/blob/main/COMMITTING.md for guidance.
I think this can be merged Wed or Thu next week assuming it all checks out.
The HTML PR is here (#8470) (though I still have a bit of updating from Domenic's feedback and I assume a few rounds of tweaks after that before it can land). The latest commit (afce7fc) has a commit message with the summary details of the change and links to the Issue and previous spec. |
Tests:
|
- Renames the request priority concept to internal priority and "adds" priority for usage by APIs. - Adds a new priority member to RequestInit for specifying a priority hint. These changes combined with whatwg/html#8470 obsolete the Priority Hints specification: https://wicg.github.io/priority-hints/. Tests: TODO. Fixes whatwg#1319. Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
The test situation still needs a bit of work, but once that is done I think this can be merged. It seems okay that HTML will be merged later as it builds on top of this. |
I have a CL up with the changes to the tests (will update here after it lands). Since the effects of priority are all implementation-specific and not exposed, other than not erroring when they are set there's not much we can test. As far as the IDL import, I pinged @foolip to see if there's a workflow to stop importing it, but I can also remove the fetch integration section from the spec which has the IDL bits (and remove the HTML part when HTML is ready to land). That should also avoid any conflicts. Main question is if I remove them before the fetch changes land or wait until they have landed and there is content to point to from the Priority Hints spec. |
IDL files in WPT come from https://github.com/w3c/webref, and there's a consistency check that runs before releasing Hope that helps? |
I decided to merge this because of:
I hope @pmeenan can sort out the WPT IDL situation and make progress on the HTML PR next. Thanks @pmeenan for your work on this feature! |
This applies the specified changes from the Priority Hints spec to fetch for #1319.
The main changes are:
priority
tointernal priority
.priority
enum to theRequest
andRequestInit
objects.Preview | Diff