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.

Sign up for GitHub

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

Jump to bottom

http: remove deprecated Client interface #8104

Merged
merged 1 commit into from
Aug 30, 2016
Merged

http: remove deprecated Client interface #8104

merged 1 commit into from
Aug 30, 2016
+13 −168

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Aug 14, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
  • http
Description of change

This commit removes the old http Client API that has been (hard) deprecated since 2011/v0.7.0.

@mscdex mscdex added http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 14, 2016
@mscdex
Copy link
Contributor Author

mscdex commented Aug 14, 2016 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/3666/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/47/
CI: https://ci.nodejs.org/job/node-test-pull-request/3668/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/48/

@mscdex mscdex force-pushed the http-remove-old-client-api branch 2 times, most recently from 5709b7d to c61dc4e Compare August 14, 2016 23:03
@mscdex
Copy link
Contributor Author

mscdex commented Aug 14, 2016

CI is green except for an unrelated failure on AIX. CITGM also looks good.

/cc @nodejs/http

@Fishrock123
Copy link
Contributor

seems fine to me if ecosystem usage is low enough

@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

LGTM

@jasnell jasnell added this to the 7.0.0 milestone Aug 15, 2016
@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

Putting on the 7.0.0 milestone

@mscdex mscdex force-pushed the http-remove-old-client-api branch from c61dc4e to 4d90e4f Compare August 26, 2016 14:23
@mscdex
Copy link
Contributor Author

mscdex commented Aug 26, 2016

/cc @nodejs/ctc Comments? Questions? Concerns? Paper? Plastic?

@jasnell
Copy link
Member

jasnell commented Aug 26, 2016

Still LGTM... now if only we can get other ctc to weigh in ;-)

@bnoordhuis
Copy link
Member

LGTM

@Fishrock123
Copy link
Contributor

@ChALkeR are you able to check npm stats on this at all?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 26, 2016

@Fishrock123 Sorry, missed it! 😞
https://gist.github.com/ChALkeR/b5105c46643373adaf6ac6cd954d991b — here it is.

@Fishrock123
Copy link
Contributor

Hmm, most of it looks pretty old to me.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 26, 2016 •
edited
Loading

242 packages. Top 10 used ones:

 432816  "node-forge"
 425178  "newrelic"
 153046  "nodeunit"
 84993  "node-rest-client"
 62103  "graphviz"
 4878  "npm-registry-couchapp"
 4732  "websocket-client"
 3981  "commonjs-everywhere"
 3596  "pn"
 3502  "felix-couchdb"

Only 5 of those have more than 5000 downloads/month.

@mscdex mscdex mentioned this pull request Aug 26, 2016
Merged
@mscdex
Copy link
Contributor Author

mscdex commented Aug 26, 2016 •
edited
Loading

  • node-forge can be dismissed, the usage I found there was actually just it creating its own http namespace with its own createClient() implementation (which isn't even documented).
  • newrelic attempts to wrap the old API, but the way it wraps them it first checks if it exists, and safely exits/returns in case the API does not exist, so that should not be a problem either.
  • nodeunit is a legit case however and I filed a PR for it here.
  • node-rest-client is also another legit case and I filed a PR for it here.
  • graphviz hasn't been updated since 2014 with unanswered issues and PRs, so IMHO I'm not sure it makes sense to try and submit a PR to fix any usage there.
  • npm-registry-couchapp is another legit case and I filed a PR for it here.
  • websocket-client hasn't been updated since 2012, so IMHO I'm not sure it makes sense to try and update that module either.
  • commonjs-everywhere similarly hasn't been updated since 2014
  • pn should be safe since it checks for existence first
  • felix-couchdb is another legit case and I filed a PR here.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2016

LGTM. Doubled bagged.

@mscdex mscdex force-pushed the http-remove-old-client-api branch from 4d90e4f to 6e16f91 Compare August 30, 2016 00:12
@mscdex
Copy link
Contributor Author

mscdex commented Aug 30, 2016 •
edited
Loading

CI one last time before landing: https://ci.nodejs.org/job/node-test-pull-request/3879/
https://ci.nodejs.org/job/node-test-pull-request/3880/

EDIT: CI is green

@mscdex
http: remove deprecated Client interface
2cc7fa5 
PR-URL: nodejs#8104
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mscdex mscdex force-pushed the http-remove-old-client-api branch from 6e16f91 to 2cc7fa5 Compare August 30, 2016 01:46
@mscdex mscdex merged commit 2cc7fa5 into nodejs:master Aug 30, 2016
@mscdex mscdex deleted the http-remove-old-client-api branch August 30, 2016 01:48
@ChALkeR ChALkeR removed their assignment Oct 23, 2016
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@mscdex @Fishrock123 @jasnell @bnoordhuis @ChALkeR @cjihrig