Skip to content

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

Closed
mscdex wants to merge 1 commit intonodejs:masterfrom
mscdex:url-empty-querystring-prototype
Closed

url: use "empty" object for empty query strings#6289
mscdex wants to merge 1 commit intonodejs:masterfrom
mscdex:url-empty-querystring-prototype

Conversation

@mscdex
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mscdex commented Apr 19, 2016

/cc @Trott

@mscdex
Copy link
Copy Markdown
Contributor Author

mscdex commented Apr 19, 2016

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

@Trott
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

mscdex commented Apr 20, 2016

Probably performance.

@mscdex
Copy link
Copy Markdown
Contributor Author

mscdex commented Apr 20, 2016

CI is green except for flaky tests.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 20, 2016

LGTM

4 similar comments
@targos
Copy link
Copy Markdown
Member

targos commented Apr 20, 2016

LGTM

@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 20, 2016

LGTM

@JungMinu
Copy link
Copy Markdown
Member

JungMinu commented Apr 20, 2016

LGTM

@cjihrig
Copy link
Copy Markdown
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 20, 2016

Landed in e9dc630

@jasnell jasnell closed this Apr 20, 2016
@jasnell
Copy link
Copy Markdown
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@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