Support for in-lined and eventemitter style callbacks about aws-sdk-js

[lsegal]

Thanks for opening this. A discussion on Twitter with @jed was recently started on this very topic. I think it should be possible to support both the primitive node callback style and the promise objects together.

[jed]

great news, @lsegal. personally, i think that providing an optional promise implementation just clutters the API, but maybe using callback omission to opt-in to a promise would work if you choose to do both.

[mjackson]

@lsegal I agree with @tracker1 that the default API should use node's standard callback interface. Those who desire a promises implementation could easily layer one on top of callback-style functions using a library such as Q.

If you'd like to include a promise implementation in the SDK, it should at least conform to CommonJS Promises/A. As it is, the current implementation in promise.js is badly broken for the following reasons:

1. The return value of a promise may be changed after callbacks have already been invoked! This makes the promise good for nothing. The reliance on the internal state variable is the problem. Instead of tracking state manually with a variable to know when to fire your callbacks, the value of the promise should be set once (i.e. the promise should be resolved) and future calls to done etc. should take their cue from that. For example, take a look at the source to rsvp.js and notice how the resolve and reject methods nuke themselves and each other to make sure they cannot be called again, thus preserving the value of the promise as it was given the first time either of them is called.
2. There is no then method on a promise, or any other method for that matter, that returns a new promise. This is the feature of promises that makes them useful. Please read You're Missing the Point of Promises for a much better explanation than I can give here in the comments as to why this feature is so key to any promise implementation.
3. The done function in this SDK means "success" but in jQuery that same term is already used to mean "resolved". This isn't nearly as big a deal as the previous two concerns, but it's still going to confuse devs who come to this SDK from jQuery-land, which will probably be very common.

Overall, I think the SDK could benefit from using a mature promises implementation like Q or rsvp.js. Both conform to CommonJS Promises/A and have plenty of real-world usage already. However, if you're intent on rolling your own implementation you could at least benefit from testing it against @domenic's promise-tests to make sure it's spec-compliant.

[mhart]

A big +1 from me and I agree with @jed and @mjijackson that there's no need to ship another promise implementation alongside the rest of the SDK. I've already run into some issues with the ways errors are transformed in the promise meaning there's no stack trace, etc.

Should make your lives a lot easier not having to support it and being able to focus on awesome AWS features 😸

[domenic]

FYI it's easy to have dual promises and callbacks with Q:

function callsBackOrReturnsPromiseForInput(input, cb) {
    return Q.resolve(input).nodeify(cb);
}

If you pass it a cb, it will call back with the usual err, result style, but otherwise it will return a promise. A real promise, that is, as @mjijackson points out.

[jed]

another reason to leave abstractions out is that there are several AWS services for which promises are sub-optimal. for example, in DynamoDB, it might make more sense for Scan and Query calls to return event emitters that emit records.

i've already deprecated my dynamo and dynamo-client libraries in favor of this one, but could see resurrecting the former once the API gels as more convenient sugar on top of this SDK.

[mikemaccana]

The existing AWS-lib module in npm already uses err first, and I can confirm it works very nicely with async.waterfall() for very common AWS tasks.

Eg, in my existing aws-lib code (which I'll probably move to the official library) I make a launch config, then an autoscale groups, then tag an autoscale group, then make a scaling policy, then make a metric alarm, then wait for the DNS record to appear. err-first is essential for tracking things in this kind of workflow.

[dmuth]

+1 I too feel that err should be the first argument, so that this module falls in line with the way things are done elsewhere in node.js.

[TomFrost]

Agreed with the above -- a promise implementation would generally be considered clutter in modern node apps. I'm definitely looking forward to refactoring my projects to depend on this, once the structure follows node convention and works with the various flow control libraries out there!

[danjenkins]

+1 on the callback/event emitter suggestions. I really don't see the point in the promise notation within nodejs, some others may disagree but I feel the primary methods of getting the data returned should be node standard and that's either an event emitter or a callback with err first and then the response data..

[trevorrowe]

I wanted to chime in and say you guys are awesome. Thank you for all of the feedback. I'm making changes to a branch that I hope to share soon. I will update here when I do.

[trevorrowe]

We've pushed a few updates to the branch "node-callbacks" (https://github.com/aws/aws-sdk-js/tree/node-callbacks). In this branch:

• all client methods accept a callback function as the last argument, as per node convention
• the first argument (params) is now optional and defaults to {}
• updated documentation examples

// without params
s3.client.listBuckets(function (err, data) {
  console.log('err', err);
  console.log('data', data);
});

// with params
s3.client.headObject({Bucket:'bucket', Key:'key'}, function (err, data) {
  console.log('err', err);
  console.log('data', data);
});

NOTE: you can still register callbacks on the returned AWSRequest object. This is especially important if you need access to the data event (e.g. when streaming files from S3).

req = s3.client.getObject({Bucket:'bucket', Key:'key'})
req.data(function (chunk) {
  // do something with the chunk of data
}).send();

If you do not use the node-style callback function when calling the operation, you must call send() on the returned request.

We still want to change the format of how you register event callbacks on the request object to match node's EventEmitter API style.

Please give it a spin and send in feedback!

FYI, to install the node-callbacks branch:

npm install [--save] 'git://github.com/aws/aws-sdk-js.git#node-callbacks'

[mhart]

Woo! That was quick! And it fills me with happiness 🐨

[dmuth]

Me too. That's awesome turnaround time! Thank you!

[mhart]

Seems to work well in my testing (against DynamoDB).

The only thing I'd add is that the err objects should really be standard JS errors/exceptions instead of the {code: '', message: ''} structures they currently are (also mentioned in #4). Either the original exception as thrown/raised in the case it came from node.js or a 3rd party lib, or one using an Error constructor (or inherited) if you need to create it yourself.

Also, for those that don't know this, you can use npm to install this branch:

npm install [--save] 'git://github.com/aws/aws-sdk-js.git#node-callbacks'

[lsegal]

Thanks @mhart. I've updated the npm install snippet in @trevorrowe's comment. We plan on tackling the error object issue when we address #4, so let's follow that there.

[danjenkins]

Awesome turnaround! I'll test it out, thanks!

[tracker1]

@trevorrowe Great work on this... Very impressed with the turn around time...

[mhart]

👍

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.