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

Allow relative URLs and http(s) scheme #37

Closed
wants to merge 1 commit into from
Closed

Allow relative URLs and http(s) scheme #37

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Contributor

@devsnek devsnek commented Oct 26, 2022 •
edited by pr-preview bot
Loading

Addresses #20.

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

This was referenced Oct 30, 2022
Closed
Closed
Jarred-Sumner added a commit to oven-sh/bun that referenced this pull request Dec 29, 2022
@Jarred-Sumner
[WebSocket] Allow http: and https: protocols
8e9af05 
See whatwg/websockets#37
@devsnek devsnek marked this pull request as ready for review January 11, 2023 09:30
@annevk annevk requested a review from ricea January 11, 2023 09:32
@ricea
Copy link
Collaborator

ricea commented Jan 20, 2023

The PR is fine, but Google is neutral on the feature. We won't support unless other browsers ship it first.

@lucacasonato
Copy link
Member

Deno would be in favor of landing this. Allowing only ws: and wss: protocols is a constant source of pain for web devs. A very simple query on GitHub Code Search reveals at least 9k candidate files in open source that have if (protocol == "https:") scheme = "wss:".

Folks, please let's ship this. It's trivial, it has tests, and it's going to make everyone's lives just slightly less terrible.

So, I'll ask for positions from WebKit and Mozilla here again: @annevk, @saschanaz, thoughts?

annevk
annevk reviewed Apr 28, 2023
View reviewed changes
index.bs
Comment on lines +132 to +135
1. If |requestURL|'s <a for=url>scheme</a> is "`ws`", set |requestURL|'s <a for=url>scheme</a>
to "`http`".
1. Otherwise, if |requestURL|'s <a for=url>scheme</a> is "`wss`", set |requestURL|'s
<a for=url>scheme</a> to "`https`".
Copy link
Member

Choose a reason for hiding this comment

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

Let's do this normalization upfront in the constructor so it doesn't leak out in a number of places.

annevk pushed a commit that referenced this pull request May 11, 2023
@devsnek @annevk
Allow HTTP(S) schemes in the WebSocket constructor
6fb220b 
And thereby relative URLs. They are instantly normalized to the ws: and wss: schemes.

Tests: web-platform-tests/wpt#39955.

Closes #37.
@annevk annevk mentioned this pull request May 11, 2023
Merged
4 tasks
@annevk
Copy link
Member

annevk commented May 11, 2023 •
edited
Loading

Since there was no follow-up, I decided to move forward with this in #45 with a slightly different approach. Hope that's okay.

@devsnek devsnek closed this May 11, 2023
@devsnek devsnek deleted the better branch May 11, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@ricea ricea Awaiting requested review from ricea

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@devsnek @ricea @lucacasonato @annevk