Conversation
lib/waterline/query/finders/basic.js
Outdated
|
|
||
| this.beforeCallbacks.call(self,criteria.where,function(err){ | ||
| if (err) {return cb(err);} | ||
| }); |
There was a problem hiding this comment.
The code flow here is so weird. Basically, we could have cb called more than once, if one of the beforeCallbacks returns an error. I know it's how they do it in the original waterline PR, but feels totally bizarre. Can't help but wonder if there's a better way to do it, but also don't want to get too far away from how they do things normally with waterline.
There was a problem hiding this comment.
Looking at how beforeDestroy and beforeUpdate work, the code flow is much nicer. They appropriately handle all async callbacks and such. It feels like the right thing to do would be to put everything after this inside the callback. That way folks could do async operations if they wanted to.
Would up the line count change significantly, but would just be indentation.
lib/waterline/query/finders/basic.js
Outdated
|
|
||
| this.beforeCallbacks.call(self,criteria.where,function(err){ | ||
| if (err) return cb(err); | ||
| }) |
lib/waterline/query/finders/basic.js
Outdated
| }); | ||
| }, | ||
|
|
||
| beforeCallbacks:function(criteria,cb){ |
There was a problem hiding this comment.
It's not clear to me why this function exists. All it does is call callbacks.beforeFind, but it's wrapped in this beforeCallbacks function that then wraps that one call in async.series. Is the idea that eventually there may be more than just beforeFind?
lib/waterline/query/finders/basic.js
Outdated
| // Normalize criteria | ||
| criteria = normalize.criteria(criteria); | ||
|
|
||
| this.beforeCallbacks.call(self,criteria.where,function(err){ |
There was a problem hiding this comment.
So, this guys first point is a problem I think. If no criteria is passed to Model.find then values will be null when it gets into the beforeFind callback.
Might need to do criteria.where || {} or something like that...not totally sure of the implications there.
There was a problem hiding this comment.
Solution suggested here balderdashy#902 (comment)
|
Functionally complete. However, closed in favor of: #3 |
2 participants
Duplicate of balderdashy#902 against current 0.10.x branch.