Skip to content

lib: remove : from protocol in Url.parse(). - #1237

Closed
anio wants to merge 1 commit into
nodejs:v1.xfrom
anio:v1.x
Closed

lib: remove : from protocol in Url.parse().#1237
anio wants to merge 1 commit into
nodejs:v1.xfrom
anio:v1.x

Conversation

@anio

@anioanio commented Mar 22, 2015

Copy link
Copy Markdown

I only removed : from parsed protocol.

@Fishrock123

Copy link
Copy Markdown
Contributor

What is the purpose of this?

@mscdex

Copy link
Copy Markdown
Contributor

If there is always a : at the end, you could avoid the regexp with lowerProto.slice(0, -1)

@Fishrock123Fishrock123 added the url Issues and PRs related to the legacy built-in url module. label Mar 22, 2015
@mscdexmscdex added semver-major PRs that contain breaking changes and should be released in the next major version. url Issues and PRs related to the legacy built-in url module. and removed url Issues and PRs related to the legacy built-in url module. labels Mar 22, 2015
@bnoordhuis

Copy link
Copy Markdown
Member

I move we just close this. It breaks all code ever written against the url module, including the test suite, for no good reason.

@kenany

Copy link
Copy Markdown
Contributor

Previously: nodejs/node-v0.x-archive#1580

Current behavior matches the browser:

(newURL('https://github.com/iojs/io.js/pull/1237')).protocol// => 'https:'

@Fishrock123

Copy link
Copy Markdown
Contributor

Might as well keep it like the browser.

@anio

anio commented Mar 23, 2015

Copy link
Copy Markdown
Author

KenanY, bnoordhuis
OK, but is it correct? I refer you to RFC 3986

@Fishrock123

Copy link
Copy Markdown
Contributor

@aino please read nodejs/node-v0.x-archive#1580

@kenany

Copy link
Copy Markdown
Contributor

@anio That describes the syntax of URLs and how they are constructed, not how to parse them. If anything, you'd have to "fix" this in the browsers first before here. Although, it's worth pointing out that the colon is part of the spec.

@anio

anio commented Mar 23, 2015

Copy link
Copy Markdown
Author

:) Thanks guys.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-majorPRs that contain breaking changes and should be released in the next major version.urlIssues and PRs related to the legacy built-in url module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@anio@Fishrock123@mscdex@bnoordhuis@kenany