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

url: use "empty" object for empty query strings #6289

Closed
wants to merge 1 commit into from
Closed

url: use "empty" object for empty query strings #6289

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 19, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • url
Description of change

This makes things consistent with the way that the querystring module creates parsed results.

@mscdex
url: use "empty" object for empty query strings
53471a4 
This makes things consistent with the way that the querystring
module creates parsed results.
@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Apr 19, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Apr 19, 2016

/cc @Trott

@mscdex
Copy link
Contributor Author

mscdex commented Apr 19, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2330/

@Trott
Copy link
Member

Trott commented Apr 20, 2016

Any reason not to just use querystring.parse() as in 60cd85577c3be73e19cf8cceb157a518d279d923? (Performance, maybe?) That way, if the implementation changes in querystring, we don't have to remember to keep url.js in sync.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 20, 2016

Probably performance.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 20, 2016

CI is green except for flaky tests.

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

LGTM

4 similar comments
@targos
Copy link
Member

targos commented Apr 20, 2016

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Apr 20, 2016

LGTM

@JungMinu
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Apr 20, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 20, 2016
@mscdex @jasnell
url: use "empty" object for empty query strings
e9dc630 
This makes things consistent with the way that the querystring
module creates parsed results.

PR-URL: #6289
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Landed in e9dc630

@jasnell jasnell closed this Apr 20, 2016
@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Marking as don't land on v4.x and v5.x because this depends on a prior semver-major change.

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@mscdex @joelostrowski
url: use "empty" object for empty query strings
079b8ee 
This makes things consistent with the way that the querystring
module creates parsed results.

PR-URL: nodejs#6289
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@mscdex @jasnell
url: use "empty" object for empty query strings
59737b2 
This makes things consistent with the way that the querystring
module creates parsed results.

PR-URL: #6289
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@emilong emilong mentioned this pull request May 10, 2016
Merged
@jasnell jasnell mentioned this pull request Jun 8, 2016
Closed
esatterwhite added a commit to node-tastypie/tastypie that referenced this pull request Jul 16, 2016
@esatterwhite
fix bug in API w/ serializer on v6. query maybe an empty object
e9b5317 
nodejs/node#6289
This pull request changes the query string to be an empty object rather
than an object inherited from Object.prototype. So it no longer has
the hasOwnProperty method
@esatterwhite esatterwhite mentioned this pull request Jul 16, 2016
Closed
@jasnell jasnell mentioned this pull request Aug 3, 2016
Closed
@ghost ghost mentioned this pull request Aug 3, 2016
Closed
@kashav kashav mentioned this pull request Aug 6, 2016
Merged
@mde mde mentioned this pull request Mar 4, 2017
Closed
@LewisJEllis LewisJEllis mentioned this pull request Mar 6, 2017
Closed
@xuewang xuewang mentioned this pull request Apr 5, 2017
Closed
@aaronjensen aaronjensen mentioned this pull request May 30, 2017
Closed
@mika-s mika-s mentioned this pull request Aug 2, 2017
Merged
cressie176 pushed a commit to guidesmiths/request-token that referenced this pull request Aug 3, 2017
Fixing broken test caused by nodejs/node#6289
95d110e
vbuyakov pushed a commit to vbuyakov/homebridge-http-notification-server that referenced this pull request Sep 4, 2018
@viktorbuyakovupw
Fix issue with query.hasOwnProperty described nodejs/node#6289
c5b831e
@vbuyakov vbuyakov mentioned this pull request Sep 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mscdex @Trott @jasnell @targos @ChALkeR @JungMinu @cjihrig