-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
docs: state decoding behavior for url pathname #1731
Conversation
Explicitly states the fact that no decoding is performed on the url path or pathname or the query string by default in the URL module. Fixes #1538
@@ -72,7 +73,8 @@ Take a URL string, and return an object. | |||
Pass `true` as the second argument to also parse the query string using the | |||
`querystring` module. If `true` then the `query` property will always be | |||
assigned an object, and the `search` property will always be a (possibly | |||
empty) string. Defaults to `false`. | |||
empty) string. If `false` then the neither the `query` nor the `search` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the superfluous 'the ' before 'neither'.
Looking good, except the typo. I think it's unnecessary to add it to |
@@ -72,7 +73,8 @@ Take a URL string, and return an object. | |||
Pass `true` as the second argument to also parse the query string using the | |||
`querystring` module. If `true` then the `query` property will always be | |||
assigned an object, and the `search` property will always be a (possibly | |||
empty) string. Defaults to `false`. | |||
empty) string. If `false` then the neither the `query` nor the `search` | |||
properties will be parsed or decoded. Defaults to `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at the code I'm not really sure search is parsed at all?
Not too familiar with url
code though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find, it looks like only query
is decoded:
> url.parse('http://foo.com/?%26', true).search
'?%26'
> url.parse('http://foo.com/?%26', true).query
{ '&': '' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, query
is the decoded from the search
field.
@silverwind removed the |
Explicitly states the fact that no decoding is performed on the url path or pathname or the query string by default in the URL module. Fixes: #1538 PR-URL: #1731 Reviewed-By: Roman Reiss <[email protected]>
Thanks, landed in a74c2c9 |
Explicitly states the fact that no decoding is performed on the url path or pathname or the query string by default in the URL module. Fixes: nodejs/node#1538 PR-URL: nodejs/node#1731 Reviewed-By: Roman Reiss <[email protected]>
Explicitly states the fact that no decoding is performed on the url
path or pathname or the query string by default in the URL module.
@Fishrock123 should I also include a similar change in the
Http.Server
IncomingMessages
documentation as mentioned in the issue?Fixes #1538