Change callback API from (err, response, data) to (err, data)

[nikulis]

• Better standardize callback format to accept two parameters,
  error and data. (BREAKING CHANGE)
• Switch from request to superagent library
• Add client-side handling of throttling requests within rate limits (via superagent-throttle plugin)
• Refactor tests to use new two-parameter API and to use mocha's promise
  API where possible (instead of manually using (done) method)
• Bump version in package.json to reflect breaking API change

[ccarterc]

Why not make response available as a 3rd parameter since it can contain helpful information such as verifying requests are hitting sandbox instead of production, headers, etc?

[rmm5t]

Why not make response available as a 3rd parameter since it can contain helpful information such as verifying requests are hitting sandbox instead of production, headers, etc?

I agree that this would make the callback interface more consistent with the promise interface , but the problem is that we have to consider backwards compatibility.

I'd be more in favor of changing the promise signature to resolve with a response object, but also injecting a response.data on that object (representing the parsed version of response.body). Nonetheless, this would also be a backwards incompatible change and would need to be reserved for a major version bump release.

Pros/Cons: exposing the raw response object in the promise gives us better ability to monitor HTTP status codes, and HTTP headers (especially for pagination), but the problem is that it then binds us to our internal HTTP implementation.

In truth, the best approach would be to return a reformatted, simpler response object that only looks like:

{
  statusCode,
  statusMessage,
  headers,
  body,
  data,  // JSON parsed version of body
}

By keeping the response signature clean like this, we don't have to worry when/if we change the underlying HTTP library, and if we need to expose more information later, we can do so in a forwards-compatible way by just adding another normalized key to the simpler response object.

Steps I'd like to see:

• Change the returned response object to match the pattern described above
• Change the promise signature to return this response object instead of just the parsed data. This is backwards incompatible, and will require clients to reference response.data instead of just data.
• Drop the 3rd argument from the callback signature such that it matches the standard node callback pattern of (err, value) (this should also make it more compatible with node features such as util.promisify() in v8).

[fb55]

I rebased this, which should have broken a bunch of stuff. In general there are a bunch of changes here that I'd love to land (the entire request refactor, including throttling, is 💯), but the callback interface should probably stay the same for now. Instead, I'd opt for deprecating the callback interface entirely and urging all users to use promises.

@rmm5t's point is very valid. At some point I'd love to switch large parts of this library to something generated from a Swagger config, having an interface in place that would allow an easy transition would be ideal. That also means we have to map superagent objects.

For me this is the last change I'd like to see land before we can publish a 1.0.

[nikulis]

I'll work on introducing the request and throttling features based on the most recent master.

I agree that the callback interface is best left alone for now.

[fb55]

@nikulis Great to see you back :D Lmk if there is anything that can be done to help you!