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: better example for http.get #9065

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@marzelin
Contributor

marzelin commented Oct 12, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

I wanted to get some JSON data using Node.js, so I searched the documentation and found http.get method that looked like a proper tool for that. But the example was confusing because of res.resume() method. My first thought was that it needs to be at the end of every http.get callback after the code for consuming the response body. But after some research I found (in the http.ClientRequest section) that it should be there only if the body won't be consumed in any other manner. But I still didn't know what the resume() method does exactly. So I search further and found that res is an instance of IncomingMessage which implements readable stream. And that's where I found description of readable.resume().

I've learnt a lot from this experience, but what I really wanted was get things done and fetch JSON.

I propose replacing current example with the one that presents the most common use of that method: getting data. Also, I added information about the type of object being passed to the callback and necessity of consuming response data in it.

doc: better example for http.get
I wanted to get some JSON data using Node.js, so I searched the documentation and found `http.get` method that looked like a proper tool for that. But the example was confusing because of `res.resume()` method. My first thought was that it needs to be at the end of  every http.get callback after the code for consuming the response body. But after some research I found (in the `http.ClientRequest` section) that it should be there only if the body won't be consumed in any other manner.  But I still didn't know what the `resume()` method does exactly. So I search further and found that `res` is an instance of `IncomingMessage` which implements readable stream. And that's where I found description of `readable.resume()`.

I've learnt a lot from this experience, but what I really wanted was get things done and fetch JSON.

I propose replacing current example with the one that presents the most common use of that method: getting data. Also, I added information about the type of object being passed to the callback and necessity of consuming response data in it.
console.log(`STATUS: ${res.statusCode}`);
res.setEncoding('utf8');
let aggregatedData = '';
res.on('data', (chunk) => aggregatedData += chunk);

This comment has been minimized.

@ChALkeR

ChALkeR Oct 12, 2016

Member

Concatenating strings isn't a good method of getting the response body.

This comment has been minimized.

@marzelin

marzelin Oct 12, 2016

Contributor

@ChALkeR what's wrong with concatenating strings, and how it should be done then?

This comment has been minimized.

@mscdex

mscdex Oct 12, 2016

Contributor

Concatenating strings is fine if the response is text, but for binary it wouldn't work. Using binary from the get-go would work for both cases (e.g. pushing Buffers to an array, keeping a total byte count, and calling Buffer.concat() at the end) but still you'd have to know what to do with the end result (either converting to string or whatever), so it's kind of tricky for a generic example.

If we want to make it more concrete, we could turn it into a JSON fetching example. That way we could show checking the response code for 200, checking that the Content-Type starts with 'application/json', and then concatenating and parsing the response?

This comment has been minimized.

@marzelin

marzelin Oct 13, 2016

Contributor

JSON fetching example sound good.

change generic example to json fetching example
As proposed in the comments, I've changed the example to present how `http.get` can be used to fetch JSON data. It also shows the use of `res.resume()` stream method.
@marzelin

This comment has been minimized.

Contributor

marzelin commented Oct 13, 2016

As proposed in the comments, I've changed the example to present how http.get can be used to fetch JSON data. It also shows the use of res.resume() stream method.

res.resume();
http.get('http://jsonplaceholder.typicode.com/posts/1', (res) => {
const statusCode = res.statusCode;
const contentType = res.headers && res.headers['content-type'];

This comment has been minimized.

@mscdex

mscdex Oct 13, 2016

Contributor

I think this can just be const contentType = res.headers['content-type'];.

This comment has been minimized.

@mscdex

mscdex Oct 13, 2016

Contributor

Also, minor nit: 2 space indent instead of 4

const contentType = res.headers && res.headers['content-type'];
let error;
if (statusCode !== 200)

This comment has been minimized.

@mscdex

mscdex Oct 13, 2016

Contributor

These if/else-ifs might look better with braces since they span multiple lines.

res.on('data', (chunk) => rawData += chunk);
res.on('end', () => {
const parsedData = JSON.parse(rawData);
console.log('Title: ' + parsedData.title);

This comment has been minimized.

@mscdex

mscdex Oct 13, 2016

Contributor

Perhaps just log the parsed value? Also, might surround JSON.parse() with a try-catch in case of parse errors.

Example:
`callback` takes one argument which is an instance of [`http.IncomingMessage`][]

This comment has been minimized.

@jasnell

jasnell Oct 13, 2016

Member

Please make this a proper sentence, e.g.:

The `callback` is invoked with a single argument that is an instance of
[`http.IncomingMessage`][]

This comment has been minimized.

@marzelin

marzelin Oct 13, 2016

Contributor

Great sentence. Thanks.

`Status Code: ${statusCode}`);
else if (!/^application\/json/.test(contentType))
error = new Error(`Invalid content-type.\n` +
`Expected application/json but received ${contentType}`);

This comment has been minimized.

@jasnell

jasnell Oct 13, 2016

Member

alignment...

        error = new Error(`Invalid content-type.\n` +
                          `Expected application/json but received ${contentType}`);
fix style and add try catch around json.parse
I've fixed spacing, corrected the callback sentence and added `try...catch` block around JSON.parse as suggested. This example is becoming more about catching errors than fetching data.
@marzelin

This comment has been minimized.

Contributor

marzelin commented Oct 13, 2016

I've fixed spacing, corrected the callback sentence and added try...catch block around JSON.parse as suggested.

@jasnell

LGTM

console.log(`Got response: ${res.statusCode}`);
// consume response body
res.resume();
http.get('http://jsonplaceholder.typicode.com/posts/1', (res) => {

This comment has been minimized.

@mscdex

mscdex Oct 14, 2016

Contributor

I wonder if we should just use a fake URL, like 'http://example.org/posts/1.json' or something? That way we have more consistency/reliability (e.g. example.org is always a reserved domain name and will never go away). The example.org url won't actually work of course, but I'm not sure that's important.

This comment has been minimized.

@Nibbler999

This comment has been minimized.

@marzelin

marzelin Oct 14, 2016

Contributor

Fetching data from nodejs.org would be great but it has to be through http. I'd prefer having a working example. jsonplaceholder.typicode.com is quite reliable and commonly used for testing and prototyping, so I'd rather keep it.

This comment has been minimized.

@lpinca

lpinca Oct 14, 2016

Member

Note that plain http also works for nodejs.org: http://nodejs.org/dist/index.json.

This comment has been minimized.

@marzelin

marzelin Oct 14, 2016

Contributor

It does work. Great 👍

This comment has been minimized.

@mscdex

mscdex Oct 14, 2016

Contributor

Let's hope that nodejs.org never forces https via redirect or similar ;-)

@lpinca

lpinca approved these changes Oct 14, 2016

LGTM

@jasnell jasnell self-assigned this Oct 17, 2016

jasnell added a commit that referenced this pull request Oct 17, 2016

doc: improved example for http.get
PR-URL: #9065
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Oct 17, 2016

Landed in cb87748. Thank you!

@jasnell jasnell closed this Oct 17, 2016

jasnell added a commit that referenced this pull request Oct 17, 2016

doc: improved example for http.get
PR-URL: #9065
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins added a commit that referenced this pull request Nov 11, 2016

doc: improved example for http.get
PR-URL: #9065
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@MylesBorins MylesBorins referenced this pull request Nov 22, 2016

Merged

v6.9.2 proposal #9735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment