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.

Sign up for GitHub

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

Jump to bottom

Process 103 Early Hints response #1404

Merged
merged 12 commits into from Apr 7, 2022
Merged

Process 103 Early Hints response #1404

merged 12 commits into from Apr 7, 2022

Conversation

bashi
Copy link
Contributor

@bashi bashi commented Feb 17, 2022 •
edited by pr-preview bot

Plumb an algorithm to process 103 Early Hints responses. The
algorithm is optional.

See whatwg/html#7598 for the overall approach.

Preview | Diff

@bashi
Process 103 Early Hints response
2616cad 
Plumb an algorithm to process 103 Early Hints responses. The
algorithm is optional.

See whatwg/html#7598 for the overall
approach.
@bashi
103 is a null body status
3316f78
@bashi bashi marked this pull request as ready for review February 17, 2022 08:40
@bashi
Copy link
Contributor Author

bashi commented Feb 17, 2022

@annevk @yutakahirano What do you think about this approach?

yutakahirano
yutakahirano reviewed Feb 18, 2022
View reviewed changes
Copy link
Member

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

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

Looks good overall. I want to have an assertion and note, as commented.

fetch.bs Outdated Show resolved Hide resolved
@whatwg whatwg deleted a comment from liuchengwei555 Feb 20, 2022
@bashi
Copy link
Contributor Author

bashi commented Feb 28, 2022

@annevk Do you think this is a reasonable approach to integrate Early Hints with the fetch standard?

annevk
annevk reviewed Feb 28, 2022
View reviewed changes
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Overall this looks reasonable to me. I have some nits and questions, and I might have some further editorial suggestions that I can also make in a fixup commit once everyone is aligned on the approach.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
bashi and others added 3 commits March 1, 2022 19:14
yoavweiss
yoavweiss reviewed Mar 1, 2022
View reviewed changes
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@yoavweiss yoavweiss mentioned this pull request Mar 1, 2022
Closed
noamr
noamr reviewed Mar 3, 2022
View reviewed changes
fetch.bs Outdated Show resolved Hide resolved
yutakahirano
yutakahirano reviewed Mar 4, 2022
View reviewed changes
fetch.bs Outdated Show resolved Hide resolved
@bashi
Copy link
Contributor Author

bashi commented Mar 4, 2022

Updated. Please let me know if I missed something.

yutakahirano
yutakahirano approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

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

LGTM

annevk
annevk reviewed Mar 10, 2022
View reviewed changes
fetch.bs Outdated
<p><a for=/>Assert</a>: <var>request</var>'s <a for=request>mode</a> is
"<code>navigate</code>".

<p class=note>Processing of early hints is only vetted for navigations.
Copy link
Member

@annevk annevk Mar 10, 2022 •
edited

Choose a reason for hiding this comment

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

This is the most significant change I made. I made it because the earlier assertion focused on "cors" while "same-origin" also exists and wasn't addressed. And it's also not clear to me that "cors" couldn't be made to work if we wanted it to work.

If this and my other changes look okay (please an explicit comment if you had a look) I think this can land after all the other requirements are met and the HTML PR is also ready to land.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for review and edit. It looks great to me.

Currently I'm adding WPTs. I'll add another comment in this PR when this could be ready to land.

Copy link
Contributor

Choose a reason for hiding this comment

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

WPTs are pretty comprehensive!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we now have good test coverage in WPT. It seems that the HTML PR (whatwg/html#7675) is getting ready to land and wants to have cross-links to this change. It would be great to merge this PR.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Mar 10, 2022
@annevk
Copy link
Member

annevk commented Apr 6, 2022

I moved the assert up and also added some information about this to the "Using fetch in other standards" chapter. If those changes look okay to @bashi and the remaining bug is filed against Safari I think this is good to go.

@bashi
Copy link
Contributor Author

bashi commented Apr 7, 2022

Thank you @annevk, the change looks great to me. I filed a bug against Safari (https://bugs.webkit.org/show_bug.cgi?id=238921).

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Apr 7, 2022
@annevk annevk merged commit 53dcb01 into whatwg:main Apr 7, 2022
@tunetheweb tunetheweb mentioned this pull request Aug 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noamr noamr noamr left review comments

@yoavweiss yoavweiss yoavweiss left review comments

@annevk annevk annevk left review comments

@yutakahirano yutakahirano yutakahirano approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bashi @annevk @noamr @yoavweiss @yutakahirano