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.

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

feat(ext/fetch): Allow Response status 101 #13969

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Conversation

ry
Copy link
Member

@ry ry commented Mar 15, 2022

Necessary for #13618

@piscisaureus
Copy link
Member

Fails the following WPT tests:

  "/fetch/api/response/response-error.any.html - Throws RangeError when responseInit's status is 100"
  "/fetch/api/response/response-error.any.html - Throws RangeError when responseInit's status is 199"      
  "/fetch/api/response/response-error.any.worker.html - Throws RangeError when responseInit's status is 100"
  "/fetch/api/response/response-error.any.worker.html - Throws RangeError when responseInit's status is 199"

If we cared for marketing reasons to pass as much WPT tests as possible, then we could just allow status code 101 or 101-198.

I think there's also a better argument to be made for allowing only 101:

  • status codes 100/102/103 are useless in our current model, because these status codes indicate that the server is going to send another response in addition to the 1xx response, but in our API there is no support for sending multiple Responses for a single Request.
  • status codes 104-199 don't exist

@bartlomieju
Copy link
Member

Fails the following WPT tests:

  "/fetch/api/response/response-error.any.html - Throws RangeError when responseInit's status is 100"
  "/fetch/api/response/response-error.any.html - Throws RangeError when responseInit's status is 199"      
  "/fetch/api/response/response-error.any.worker.html - Throws RangeError when responseInit's status is 100"
  "/fetch/api/response/response-error.any.worker.html - Throws RangeError when responseInit's status is 199"

If we cared for marketing reasons to pass as much WPT tests as possible, then we could just allow status code 101 or 101-198.

I think there's also a better argument to be made for allowing only 101:

  • status codes 100/102/103 are useless in our current model, because these status codes indicate that the server is going to send another response in addition to the 1xx response, but in our API there is no support for sending multiple Responses for a single Request.
  • status codes 104-199 don't exist

Good point, I think allowing only 101 is a good compromise, but we should update info in MDN compat table too (CC @lucacasonato )

@lucacasonato
Copy link
Member

I need to review the implications of this in regards to spec.

@bartlomieju bartlomieju changed the title Allow Response status 101 feat(ext/web): Allow Response status 101 Mar 15, 2022
@bartlomieju bartlomieju changed the title feat(ext/web): Allow Response status 101 feat(ext/fetch): Allow Response status 101 Mar 15, 2022
@bartlomieju
Copy link
Member

This PR allows to ship Deno.upgradeHttp #13618

@bartlomieju bartlomieju merged commit 45b3aa2 into denoland:main Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants