-
Notifications
You must be signed in to change notification settings - Fork 30k
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: fixed ambiguity in http.md and https.md #48692
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence removed ("Properties that are inherited from the prototype are ignored.") is actually correct (see #48688 (comment)) and was added in e1161a3.
Anyway I'm fine with removing it to avoid confusion.
You might also want to remove "sets the method to GET and" from the sentence
The only difference between this method and `http.request()` is that it sets the
method to GET and calls `req.end()` automatically.
a few lines below.
Also please do the same for |
I think this sentence is correct
|
You're right, brother. I'll do it |
It doesn't set the method to GET, it use options object. If the const http = require('http');
const server = http.createServer();
server.on('request', function (request, response) {
console.log(request.method); // Prints: POST
response.end('OK');
});
server.listen(function () {
const { port } = server.address();
const request = http.get(`http://localhost:${port}`, { method: 'POST' });
request.on('response', function (response) {
response.resume();
response.on('end', function () {
server.close();
});
});
}); |
Can you please remove the merge commit? |
Yeah, i did it |
I see a merge commit here https://github.com/nodejs/node/pull/48692/commits. |
I fixed it |
598f56a
to
cb9d30a
Compare
I apologize for modifying my changes to meet the requirements. Please review them again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 8610be7. |
PR-URL: nodejs#48692 Fixes: nodejs#48688 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#48692 Fixes: nodejs#48688 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
#48688 (comment)