Skip to content

Conversation

@SamuelMarks
Copy link

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Docs for HTTP GET shortcut method https://nodejs.org/api/http.html#http_http_get_options_callback

Not filling out rest of PR requirements yet, just an RFC for now.

function shortcutMethod(method) {
return function(options, cb) {
options['method'] = method;
var req = exports.request(options, cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

what if you want to write to the request? Like on a POST or PUT?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, in my current code I have
'Content-Length': body ? Buffer.byteLength(body) : 0,

Then later on:
body && req.write(body);

But for now just want to get some sort of idea if people even want this new shortcut feature implemented.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Apr 1, 2016
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 1, 2016
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

Hmm.. I understand the reasoning but I'm leaning towards -1 on this mainly because there are plenty of userland options for handling this kind of thing. /cc @nodejs/http

@mscdex
Copy link
Contributor

mscdex commented Apr 1, 2016

-1 from me as well

@evanlucas
Copy link
Contributor

I agree with @jasnell and @mscdex. Thanks for the contribution, but I'm not sure this is something we want to pursue.

@indutny
Copy link
Member

indutny commented Apr 1, 2016

-1 from me as well, sorry!

@cjihrig
Copy link
Contributor

cjihrig commented Apr 1, 2016

Thanks for the PR. I'm going to close in light of all the -1s.

@cjihrig cjihrig closed this Apr 1, 2016
@SamuelMarks
Copy link
Author

No worries. Yeah, I suspected you guys didn't want it, which is why I did the RFC + didn't provide a full PR.

FYI: For others wanting to use this approach, here's a working gist that presents with imports from the https module in JavaScript and TypeScript - https://gist.github.com/SamuelMarks/d561575d13499c53ebead97e0a51eb04

@SamuelMarks SamuelMarks deleted the patch-1 branch April 7, 2016 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants