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

Undocumented url.parse pathname change #30154

Closed
GrosSacASac opened this issue Oct 28, 2019 · 6 comments
Closed

Undocumented url.parse pathname change #30154

GrosSacASac opened this issue Oct 28, 2019 · 6 comments
Labels
doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.

Comments

@GrosSacASac
Copy link
Contributor

Code

var url = require("url")
console.log(url.parse('ws://localhost').pathname)

Node version and output

12.13.0
"/"
10.16.3
null

The change is not visible in the docs

I could submit a PR if you confirm this should be added to the doc
@targos
Copy link
Member

targos commented Oct 28, 2019

The result changed between v11.13.0 and v11.14.0.

Commit: 8798db3

@GrosSacASac
Copy link
Contributor Author

Should this change be documented ?

@targos
Copy link
Member

targos commented Oct 28, 2019

/cc @nodejs/url

@ZYSzys
Copy link
Member

ZYSzys commented Oct 28, 2019

Should this change be documented ?

@GrosSacASac Maybe yes. I think it's better than nothing.

@lpinca
Copy link
Member

lpinca commented Nov 10, 2019 •
edited
Loading

It is a bug fix. I don't think the change should be documented because it does not make much sense to document that for this special case a wrong pathname value is returned for ws: and wss: on Node.js <= 11.13.0.

I think it makes more sense to backport the fix. I did not do it because it could be seen as a semver-major change.

@devsnek
Copy link
Member

devsnek commented Nov 10, 2019

this should be added to the history section of the api

@devsnek devsnek added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Nov 10, 2019
@lpinca lpinca mentioned this issue Nov 10, 2019
Closed
3 tasks
lpinca added a commit to lpinca/node that referenced this issue Nov 10, 2019
@lpinca
doc: add entry to url.parse() changes metadata
77885ff 
Document that returned `pathname` is now `/` when the URL to parse has
no path and the protocol scheme is`ws:` or `wss:`.

Fixes: nodejs#30154
Refs: nodejs#26941
@lpinca lpinca closed this as completed in 04e45db Nov 15, 2019
MylesBorins pushed a commit that referenced this issue Nov 17, 2019
@lpinca @MylesBorins
doc: add entry to url.parse() changes metadata
e4a296c 
Document that returned `pathname` is now `/` when the URL to parse has
no path and the protocol scheme is`ws:` or `wss:`.

PR-URL: #30348
Fixes: #30154
Refs: #26941
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Dec 1, 2019
@lpinca @targos
doc: add entry to url.parse() changes metadata
1ef1f4f 
Document that returned `pathname` is now `/` when the URL to parse has
no path and the protocol scheme is`ws:` or `wss:`.

PR-URL: #30348
Fixes: #30154
Refs: #26941
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
@lpinca @MylesBorins
doc: add entry to url.parse() changes metadata
d979a9d 
Document that returned `pathname` is now `/` when the URL to parse has
no path and the protocol scheme is`ws:` or `wss:`.

PR-URL: #30348
Fixes: #30154
Refs: #26941
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

doc: add entry to url.parse() changes metadata lpinca/node
5 participants
@lpinca @targos @GrosSacASac @devsnek @ZYSzys