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.

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

Add success and error methods to query chains. #316

Closed
ptnplanet opened this issue Aug 26, 2013 · 4 comments
Closed

Add success and error methods to query chains. #316

ptnplanet opened this issue Aug 26, 2013 · 4 comments

Comments

@ptnplanet
Copy link

I am tired of writing

if (err) { return cb(err); }

on the first line in all of my callbacks. It's redundant code. ORM itself uses this line 50 times.

I would like to suggest adding an EventEmitter or Promise architecture to ORM that provides an api similar to this widely used approach:

EventEmitter.success(function (data) { /* ... */ });
EventEmitter.error(function (err) { /* ... */ });
EventEmitter.complete(function (err, data) { /* ... */ });

So instead of writing

Model.find(/* ... */).run(function (err, results) {
    if (err) { return errorCallback(err); }
    /* do something with results */
});

with error handling in all of the callbacks, its much nicer to write

Model.find(/* ... */).success(function (results) {
    /* do something with results */
}).error(errorCallback);

and let a dedicated errorCallback handle the error.

Now, there are two possible ways on integrating this. Option one is to add success and error functions to the chains and proxy their run method. Option two is to add a dedicated EventEmitter to ORM and replace basically all occurrences of return cb(/* ... */) with something along the line of

return new EventEmitter(function (emitter) {
    /* do something that might cause errors */
    if (err) {
        emitter.emit('error', err);
    } else {
        emitter.emit('success', results);
    }
});

Actually, this would only have to be done on the driver and other lower levels. Higher levels can adapt to the api of the returned event emitters.

What do you guys think? Maybe something for 3.x? I'm happy to contribute.

@dresende
Copy link
Owner

Will look into this :)

dresende added a commit that referenced this issue Aug 26, 2013
This is just an initial code, we should improve in the future and make
more parts of the code use the "Promise" (like Model.get)
@dresende
Copy link
Owner

This is just a mockup, will probably change a lot, but the user API will probably be this one, maybe some more useful methods, I don't know..

@ptnplanet
Copy link
Author

Looks good to me. The only thing I would like to add here is, that when starting to implement this style of promise syntax, it is worth considering to add a complete Promise and event architecture. This would allow to chain promises like

Model.find().success(cb1).error(cb2).complete(cb3);

, enable data flow through callbacks

Model.find().success(cb1).then(cb2).then(cb3);

, or even stuff like this:

Promise.when(Model.find(), Model.find(), Model.find(), cb);

As I already said, these are just ideas for future versions.

@dresende
Copy link
Owner

Yes, that is something desirable. We'll continue to improve the lib to use this globally and enhance the funcionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants