Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

`node v0.9.x` compatibility #399

Closed
wants to merge 5 commits into from

5 participants

@mmalecki
Collaborator

This isn't ready to merge yet, merely heads up for everyone wanting to use request with unstable version of node.

Right now, streams2 compatibility is included. rejectUnauthorized in tests should still be changed.

@mmalecki
Collaborator

7af1083 and fed0edb fixed most of the https tests. Still trying to figure out test-httpModule.

@mmalecki
Collaborator

Can we consider this for merging? cc @mikeal @isaacs

@isaacs

I think if strictSSL is set explicitly, then rejectUnauthorized should be set that way as well. Maybe just deprecate that option, and guide people towards using rejectUnauthorized instead?

@mikeal
Owner

this no longer merges cleanly.

@mikeal
Owner

this doesn't merge cleanly, please reopen when it does.

@mikeal mikeal closed this
nrn and others added some commits
@nrn nrn Merge branch 'master' of github.com:mikeal/request b4c4c28
@mikeal mikeal Merge pull request #454 from mafintosh/master
Destroy the response if present when destroying the request (clean merge)
2242328
@mikeal mikeal Merge pull request #429 from nrn/master
Copy options before adding callback.
049cefa
@mmalecki mmalecki Call `response.resume()` to start the data flow
This fixes tests which were broken since introducing `streams2`.
4a26bf6
@mmalecki
Collaborator

@mikeal this now merges cleanly, please reopen

@mikeal mikeal reopened this
@mikeal
Owner

if this doesn't break 0.8 i could just merge it now :)

@mmalecki
Collaborator

This doesn't break 0.8 - tests still pass.

@kenperkins kenperkins commented on the diff
@@ -1140,6 +1141,8 @@ function request (uri, options, callback) {
options = uri
}
+ options = copy(options)

What's the reason to move options copy down?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mikeal mikeal closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 6, 2013
  1. @nrn
Commits on Mar 1, 2013
  1. @nrn
Commits on Mar 2, 2013
  1. @mikeal

    Merge pull request #454 from mafintosh/master

    mikeal authored
    Destroy the response if present when destroying the request (clean merge)
  2. @mikeal

    Merge pull request #429 from nrn/master

    mikeal authored
    Copy options before adding callback.
  3. @mmalecki

    Call `response.resume()` to start the data flow

    mmalecki authored
    This fixes tests which were broken since introducing `streams2`.
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 2 deletions.
  1. +4 −1 index.js
  2. +2 −1  tests/test-params.js
View
5 index.js
@@ -108,7 +108,6 @@ function Request (options) {
}
}
}
- options = copy(options)
this.init(options)
}
@@ -572,6 +571,8 @@ Request.prototype.start = function () {
delete reqOptions.auth
self.req = self.httpModule.request(reqOptions, function (response) {
+ response.resume();
+
if (response.connection.listeners('error').indexOf(self._parserErrorHandler) === -1) {
response.connection.once('error', self._parserErrorHandler)
}
@@ -1140,6 +1141,8 @@ function request (uri, options, callback) {
options = uri
}
+ options = copy(options)

What's the reason to move options copy down?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
if (callback) options.callback = callback
var r = new Request(options)
return r
View
3  tests/test-params.js
@@ -82,7 +82,8 @@ s.listen(s.port, function () {
}
counter = counter - 1;
if (counter === 0) {
- console.log(Object.keys(tests).length+" tests passed.")
+ assert.notEqual(typeof test.callback, 'function')
+ console.log(1 + Object.keys(tests).length+" tests passed.")
s.close()
}
})
Something went wrong with that request. Please try again.