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.

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

doc: res.getHeaders return value wrong in docs #12885

Closed
refack opened this issue May 7, 2017 · 7 comments
Closed

doc: res.getHeaders return value wrong in docs #12885

refack opened this issue May 7, 2017 · 7 comments
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.

Comments

@refack
Copy link
Contributor

refack commented May 7, 2017

  • Version: 7
  • Platform: *
  • Subsystem: HTTP

Ref: https://github.com/nodejs/node/pull/12883/files#r115149721

      const headersCopy = res.getHeaders();
      assert.ok(headersCopy instanceof Object);

Fails with AssertionError [ERR_ASSERTION]: false == true
In contradiction to API https://nodejs.org/api/http.html#http_response_getheaders

@refack
Copy link
Contributor Author

refack commented May 7, 2017

More concretely
headersCopy.hasOwnProperty === undefined
&
headersCopy.propertyIsEnumerable === undefined

@refack refack added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels May 7, 2017
@addaleax addaleax removed the test Issues and PRs related to the tests. label May 7, 2017
@addaleax
Copy link
Member

addaleax commented May 7, 2017

See #12883 (comment) – can please you be precise about what the issue is or what you want to do here? Otherwise this issue is just stating facts.

@refack
Copy link
Contributor Author

refack commented May 7, 2017

If there is a reason why it has a null prototype, we need to correct the API documentation.
Else just give it a prototype.

@addaleax
Copy link
Member

addaleax commented May 7, 2017

If there is a reason why it has a null prototype

Yes, because header names can clash with the built-in fields of Object.prototype (for example with __proto__ itself).

correct the API

Are you talking about docs?

@refack
Copy link
Contributor Author

refack commented May 7, 2017

Are you talking about docs?

Yes

@addaleax addaleax added the doc Issues and PRs related to the documentations. label May 7, 2017
@mscdex
Copy link
Contributor

mscdex commented May 7, 2017

You can probably just copy the text from querystring.parse().

refack added a commit to refack/node that referenced this issue May 8, 2017
* also correct language for the same note for querystring.parse
* add assertions for said note

Fixes: nodejs#12885
@refack refack changed the title http: res.getHeaders return value not an Object doc: res.getHeaders return value wrong in docs May 8, 2017
refack added a commit to refack/node that referenced this issue May 10, 2017
* also correct language for the same note for querystring.parse
* add assertions for said note

PR-URL: nodejs#12887
Fixes: nodejs#12885
Refs: nodejs#12883
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@refack
Copy link
Contributor Author

refack commented May 10, 2017

Fixed by: #12887

@refack refack closed this as completed May 10, 2017
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
* also correct language for the same note for querystring.parse
* add assertions for said note

PR-URL: nodejs#12887
Fixes: nodejs#12885
Refs: nodejs#12883
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants