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

Decision to use node cb style for lifecycle hooks #176

Closed
clark-pan opened this issue Sep 16, 2014 · 3 comments
Closed

Decision to use node cb style for lifecycle hooks #176

clark-pan opened this issue Sep 16, 2014 · 3 comments

Comments

@clark-pan
Copy link

Is there a specific reason why you chose to use the nodejs cb style for the lifecycle hook functions?

Since we've got promises anyways, and internally it seems like you convert them into promises (by decorating $q, which isn't a very good idea) why not just wrap the return value in a $q.when?

So instead of:

promise
  .then(function (attrs) {
    return DS.$q.promisify(definition.beforeValidate)(resourceName, attrs);
  })

why not:

promise
  .then(function(attrs){
    return definition.beforeValidate(resourceName, attrs);
  })

It would provide consistency in within the library and application code in how async is handled.

@jmdobry
Copy link
Member

jmdobry commented Sep 16, 2014

My original reasoning? I wanted the hooks to be asynchronous, but I didn't want to force developers to create promises. By using callbacks, developers don't have to wrap any callback-based api they might use in a hook with a new promise. I didn't want to just assume that devs use $q for everything and only use promise-based code. Using $q.when precludes use of callbacks. Callbacks do not preclude the use of promises.

Right now I am in the process of making angular-data usable in any framework, in which case I certainly cannot rely on devs being able to use $q.

I do agree however, that the api should be more consistent. You should be able to just return a promise inside a lifecycle hook if you don't use the callback. PR?

@clark-pan
Copy link
Author

@jmdobry Done

@jmdobry
Copy link
Member

jmdobry commented Sep 16, 2014

Fixed by #177

@jmdobry jmdobry closed this as completed Sep 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants