vadimdemedes / mongorito

🍹 MongoDB ODM for Node.js apps based on Redux
1.38k stars 90 forks source link

allow .then on Query #144

Closed EduardoRFS closed 7 years ago

EduardoRFS commented 8 years ago

When execute mquery methods on Query class, not have .then method

before:

Post.where('_id', 1).then() // error non function
Post.findOneAndUpdate(...).then() // error non function

after:

Post.where('_id', 1).then() // Promise
Post.findOneAndUpdate(...).then() // Promise

await Post.findOneAndUpdate(...) // works
coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 95.349% when pulling 875cba1d7f44d23ab538c0aacb4da1b1744a85ca on EduardoRFS:v3-fix-query into 4b6e8d2fcccba40411be5e167aea3c9ecc31ee5b on vdemedes:v3.

vadimdemedes commented 8 years ago

Good suggestion, but to make .catch() work, I'd implement that like:

then() {
  return new Promise((resolve, reject) => {
    return new this.model.dbCollection()
      .then(collection => this.mquery.collection(collection))
      .then(resolve, reject);
  });
}

Also, do you mind adding a test to confirm it's working?

EduardoRFS commented 8 years ago

@vdemedes refactored Query, removed unnecessary code and allowed syntax like .findOne().where(...), .catch is after .then ex: .findOne().then(...).catch(...). Basically with this PR Query is a instanceof mquery, with custom .then all tests use .then

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-3.2%) to 90.252% when pulling 5939f546bb39f869f64dc9862166683b48bd1bef on EduardoRFS:v3-fix-query into ccf061bc131038dce788e1cc5d5b992d84a5c887 on vdemedes:v3.

vadimdemedes commented 8 years ago

Could you revert this PR back to its original point, without making Query extend mquery?

In future, please consider opening an issue or discussing in the PR first, before making major changes. That way, you avoid extra work that might be rejected and you don't waste your precious time ;)

EduardoRFS commented 8 years ago

Reverted, i make that to use in a project. Is only a suggestion

zaaack commented 7 years ago

Recently I hit this issue too, at first I expose mquery instance and implement then method to using findOneAndUpdate, then I realize the return is just like the native driver, to using like mongorito still need write hooks and wrap, like an wrap method of query, other hand the dbCollection is enough.

vadimdemedes commented 7 years ago

Closing, turns out this is a bad practice, see https://github.com/vadimdemedes/mongorito/issues/176.