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

Closed
wants to merge 4 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 33 additions & 8 deletions doc/api/http.md
Expand Up @@ -1354,18 +1354,43 @@ added: v0.3.6
-->

Since most requests are GET requests without bodies, Node.js provides this
convenience method. The only difference between this method and [`http.request()`][]
is that it sets the method to GET and calls `req.end()` automatically.
convenience method. The only difference between this method and
[`http.request()`][] is that it sets the method to GET and calls `req.end()`
automatically. Note that response data must be consumed in the callback
for reasons stated in [`http.ClientRequest`][] section.

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

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great sentence. Thanks.


JSON Fetching Example:

```js
http.get('http://www.google.com/index.html', (res) => {
console.log(`Got response: ${res.statusCode}`);
// consume response body
res.resume();
http.get('http://jsonplaceholder.typicode.com/posts/1', (res) => {
Copy link
Contributor

@mscdex mscdex Oct 14, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does work. Great 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const statusCode = res.statusCode;
const contentType = res.headers && res.headers['content-type'];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, minor nit: 2 space indent instead of 4


let error;
if (statusCode !== 200)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

alignment...

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

if (error) {
console.log(error.message);
// consume response data to free up memory
res.resume();
return;
}

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

Choose a reason for hiding this comment

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

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

});
}).on('error', (e) => {
console.log(`Got error: ${e.message}`);
console.log(`Got error: ${e.message}`);
});
```

Expand Down