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, test: add note to response.getHeaders #12887

Closed
wants to merge 0 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented May 7, 2017

  • also correct language for the same note for querystring.parse
  • add assertions for said note

Fixes: #12885
Ref: #12883

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test,doc

Sorry, something went wrong.

@refack refack requested a review from mscdex May 7, 2017 18:51
@refack refack self-assigned this May 7, 2017
@refack refack added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels May 7, 2017
@refack
Copy link
Contributor Author

refack commented May 7, 2017

prototypically extend from the JavaScript `Object`. This means that the
typical `Object` methods such as `obj.toString()`, `obj.hasOwnProperty()`,
and others are not defined and *will not work*.
prototypically extend the JavaScript `Object`. This means that typical`Object`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: space after typical

and others are not defined and *will not work*.
prototypically extend the JavaScript `Object`. This means that typical`Object`
methods such as `obj.toString()`, `obj.hasOwnProperty()`, and others are not
defined and *will not work*. As well `obj instanceof Object === false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to use complete sentences in the documentation? Also, this isn’t that simple anyway. Other Contexts can have different Object objects anyway – the better advice is to avoid instanceof for builtins in JS code (i.e. please drop the last sentence here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable 👍

doc/api/http.md Outdated
@@ -974,7 +974,11 @@ const headerNames = response.getHeaderNames();
added: v7.7.0
-->

* Returns: {Object}
* Returns: {Object}
*Note*: The object returned by the `response.getHeaders()` method _does not_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this note at the end of the documentation section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only difference is that there there is no explicit * Returns: {Object}

doc/api/http.md Outdated
@@ -1,4 +1,4 @@
# HTTP
# HTTP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on with this hunk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, reverting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a weird space at line 559 that made my editor decide it's a UTF-8 file, and added the BOM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean UTF-16?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc/api/http.md Outdated
@@ -974,7 +974,11 @@ const headerNames = response.getHeaderNames();
added: v7.7.0
-->

* Returns: {Object}
* Returns: {Object}
*Note*: The object returned by the `response.getHeaders()` method _does not_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put the note in the prose like querystring.parse()?

doc/api/http.md Outdated
* Returns: {Object}
* Returns: {Object}
*Note*: The object returned by the `response.getHeaders()` method _does not_
prototypically extend the JavaScript `Object`. This means that typical`Object`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space between "typical" and "Object"

doc/api/http.md Outdated
*Note*: The object returned by the `response.getHeaders()` method _does not_
prototypically extend the JavaScript `Object`. This means that typical`Object`
methods such as `obj.toString()`, `obj.hasOwnProperty()`, and others are not
defined and *will not work*. As well `obj instanceof Object === false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either make the last bit a complete sentence, or fold it into the previous sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing.

// eslint-disable-next-line no-restricted-properties
assert.deepEqual(res.getHeaders(), {});
assert.deepEqual(headers, {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deepStrictEqual while at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove prototype from {}, but why not...

@refack refack force-pushed the fix-12885-change-doc branch from 178a714 to 1bc296e Compare May 7, 2017 19:16
@refack
Copy link
Contributor Author

refack commented May 7, 2017

Addressed. PTAL.

@refack
Copy link
Contributor Author

refack commented May 7, 2017

doc/api/http.md Outdated
@@ -982,6 +982,11 @@ header-related http module methods. The keys of the returned object are the
header names and the values are the respective header values. All header names
are lowercase.

*Note*: The object returned by the `response.getHeaders()` method _does not_
prototypically extend the JavaScript `Object`. This means that typical`Object`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still a missing space after “typical”

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

doc/api/http.md Outdated
@@ -982,6 +982,11 @@ header-related http module methods. The keys of the returned object are the
header names and the values are the respective header values. All header names
are lowercase.

*Note*: The object returned by the `response.getHeaders()` method _does not_
prototypically extend the JavaScript `Object`. This means that typical`Object`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing a space.

assert.deepEqual(res.getHeaders(), {});
const headers = res.getHeaders();
const objLike = Object.create(Object.getPrototypeOf(headers));
assert.deepStrictEqual(headers, objLike);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just compare it to Object.create(null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails on 7.10 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.
(Well the test fails anyway in other places on 7.10)

@refack refack force-pushed the fix-12885-change-doc branch 2 times, most recently from 1677823 to f13cde2 Compare May 7, 2017 19:25
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the old querystring.md wording sounded a bit clearer to me.

@refack
Copy link
Contributor Author

refack commented May 7, 2017

/cc @nodejs/documentation comments about the wording?

doc/api/http.md Outdated
@@ -982,6 +982,11 @@ header-related http module methods. The keys of the returned object are the
header names and the values are the respective header values. All header names
are lowercase.

*Note*: The object returned by the `response.getHeaders()` method _does not_
prototypically extend the JavaScript `Object`. This means that typical `Object`
Copy link
Contributor

@mscdex mscdex May 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/extend the/inherit from the/

doc/api/http.md Outdated
@@ -556,7 +556,7 @@ Returns `request`.
added: v0.1.17
-->

This class inherits from [`net.Server`][] and has the following additional events:
Thisclass inherits from [`net.Server`][] and has the following additional events:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure, the extra whitespace at the end of this line has been removed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a Unicode white space there, that I converted to \u0020. I'll make sure that it's right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a \0x00A0 at position 5 I replaced with \u0020. I verified with a hex compare.

// eslint-disable-next-line no-restricted-properties
assert.deepEqual(res.getHeaders(), {});
const headers = res.getHeaders();
const objLike = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is an object, not object like :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this discussion with @addaleax. How can it be an Object if objLike instaceof Object === false ?
According to OOP definitions "not" objLike is an Object. It's actually a null with properies 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could call it sonOfNull...

Copy link
Contributor

@mscdex mscdex May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @thefourtheye, it's still an object according to the spec.

@refack refack force-pushed the fix-12885-change-doc branch from f13cde2 to b303aef Compare May 8, 2017 02:53
doc/api/http.md Outdated
@@ -556,7 +556,7 @@ Returns `request`.
added: v0.1.17
-->

This class inherits from [`net.Server`][] and has the following additional events:
This class inherits from [`net.Server`][] and has the following additional events:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced a \u00A0 with a \u0020, so now the document is ANSI and not UTF-8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack Just to be clear, our docs are UTF-8 and don’t need to be restricted to ASCII. This change is obviously fine, but it might be a good idea to tell your editor to not use BOMs for UTF-8.

prototypically extend from the JavaScript `Object`. This means that the
typical `Object` methods such as `obj.toString()`, `obj.hasOwnProperty()`,
and others are not defined and *will not work*.
prototypically extend the JavaScript `Object`. This means that typical `Object`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/extend the/inherit from the/

@refack refack force-pushed the fix-12885-change-doc branch from b303aef to f16669a Compare May 8, 2017 03:11
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(res.getHeaders(), {});
const headers = res.getHeaders();
const exoticObj = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I am being extremely pedantic here. So please ignore this comment, if you don't agree. Quoting 9.1 Ordinary Object Internal Methods and Internal Slots,

All ordinary objects have an internal slot called [[Prototype]]. The value of this internal slot is either null or an object and is used for implementing inheritance.

This object doesn't do or posses anything exotic. So this is still a normal object I believe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @thefourtheye. Probably better to call it something like simpleObj or bareObj or something similar without getting too wordy...

@refack refack closed this May 10, 2017
@refack refack force-pushed the fix-12885-change-doc branch from f16669a to e1cabf6 Compare May 10, 2017 15:08
refack added a commit to refack/node that referenced this pull request 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 <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@refack
Copy link
Contributor Author

refack commented May 10, 2017

landed in e1cabf6

@refack refack deleted the fix-12885-change-doc branch May 10, 2017 15:09
@refack
Copy link
Contributor Author

refack commented May 10, 2017

@mscdex
Copy link
Contributor

mscdex commented May 10, 2017

@refack I think you left that CI comment on the wrong PR? That CI run is for this commit: "test: use assert regexp in tls no cert test"

@refack
Copy link
Contributor Author

refack commented May 10, 2017

@refack I think you left that CI comment on the wrong PR? That CI run is for this commit: "test: use assert regexp in tls no cert test"

Since the CI is on master the one CI job covers three small lands I did almost together.

anchnk pushed a commit to anchnk/node that referenced this pull request 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 <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: res.getHeaders return value wrong in docs
7 participants