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

Upper case all http methods in fetch API with compat flag #2913

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 11, 2024

This introduces a non-standard behavior that would become the default. Please review carefully. See the original bug report for context (linked below).

Fixes: #2516

@jasnell jasnell requested review from a team as code owners October 11, 2024 17:50
@jasnell jasnell changed the title Eliminate an unnecessary string copy Upper case all http methods in fetch API with compat flag Oct 11, 2024
src/workerd/api/http.c++ Show resolved Hide resolved
@kentonv
Copy link
Member

kentonv commented Oct 11, 2024

Up to you but personally I'd be in favor of not bothering with a compat flag here and just making the change for everyone. It only turns an error case into a non-error case and I can't imagine why anyone would be probing for this error.

@jasnell
Copy link
Member Author

jasnell commented Oct 11, 2024

I'd prefer the compat flag if only to provide a means of reverting back to the more strict spec-compliance mode. I'm not a fan of intentionally moving away from the spec requirements without at least providing a safety hatch.

This introduces a non-standard behavior to uppercase *all* HTTP methods
in fetch requests rather than just the limited set defined by the standard.

Fixes: #2516
@jasnell jasnell force-pushed the jsnell/uppercase-all-http-methods branch from 2e443f5 to 077a0a7 Compare October 11, 2024 19:34
@jasnell jasnell merged commit 6625e76 into main Oct 11, 2024
13 checks passed
@jasnell jasnell deleted the jsnell/uppercase-all-http-methods branch October 11, 2024 19:45
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.

🐛 Bug Report — Request Throws 'Invalid HTTP method' Error when Using Lowercase Method "patch"
3 participants