doc: better example for http.get by marzelin · Pull Request #9065 · nodejs/node

marzelin · 2016-10-12T20:17:30Z

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.

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.

ChALkeR · 2016-10-12T20:36:21Z

doc/api/http.md

+  console.log(`STATUS: ${res.statusCode}`);
+  res.setEncoding('utf8');
+  let aggregatedData = '';
+  res.on('data', (chunk) => aggregatedData += chunk);


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

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

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?

JSON fetching example sound good.

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 · 2016-10-13T10:15:30Z

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.

mscdex · 2016-10-13T12:05:16Z

doc/api/http.md

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


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

Also, minor nit: 2 space indent instead of 4

mscdex · 2016-10-13T12:06:29Z

doc/api/http.md

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


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

mscdex · 2016-10-13T12:08:07Z

doc/api/http.md

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


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

jasnell · 2016-10-13T16:07:31Z

doc/api/http.md

+for reasons stated in [`http.ClientRequest`][] section.

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


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

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

Great sentence. Thanks.

jasnell · 2016-10-13T16:08:55Z

doc/api/http.md

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


alignment...

error = new Error(`Invalid content-type.\n` + `Expected application/json but received ${contentType}`);

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 · 2016-10-13T18:08:05Z

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

jasnell

LGTM

mscdex · 2016-10-14T16:47:43Z

doc/api/http.md

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


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.

Could point to https://nodejs.org/dist/index.json

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.

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

It does work. Great 👍

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

lpinca

LGTM

PR-URL: #9065 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

jasnell · 2016-10-17T16:04:13Z

Landed in cb87748. Thank you!

PR-URL: #9065 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Oct 12, 2016

ChALkeR reviewed Oct 12, 2016

View reviewed changes

change generic example to json fetching example

dd28020

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.

mscdex reviewed Oct 13, 2016

View reviewed changes

jasnell reviewed Oct 13, 2016

View reviewed changes

fix style and add try catch around json.parse

1b4c0f8

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.

jasnell approved these changes Oct 14, 2016

View reviewed changes

mscdex reviewed Oct 14, 2016

View reviewed changes

lpinca approved these changes Oct 14, 2016

View reviewed changes

change url to nodejs.org host

918bb38

jasnell self-assigned this Oct 17, 2016

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

doc: improved example for http.get

cb87748

PR-URL: #9065 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

jasnell closed this Oct 17, 2016

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

doc: improved example for http.get

95d45d7

PR-URL: #9065 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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

doc: improved example for http.get

2c5b27a

PR-URL: #9065 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins added dont-land-on-v4.x labels Nov 11, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v6.9.2 proposal #9735

Merged

Uh oh!

Conversation

marzelin commented Oct 12, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marzelin commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marzelin commented Oct 13, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mscdex Oct 12, 2016 •

edited

Loading

mscdex Oct 14, 2016 •

edited

Loading