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

Added Datastore.findOpts(query, opts, callback) #489

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

iyobo
Copy link

@iyobo iyobo commented Jan 15, 2017

findOpts is an extended version of the find function, where the 2nd parameter (previously just projections) is now used to pass in fields (aka projections), paging, and sort values which, for the most part, eliminates the need to return a cursor.

This function will never return a cursor and will always expect the last parameter to be a callback, keeping it auto-promisification compliant.

i.e. With this function, you can now run promise.promisifyAll(...) on an instance of Datastore to be able to use await or yield on findOptsAsync with all working find features whereas simply using findAsync function would be severely handicapped as it would not be able to page or sort.

@PopovMP
Copy link

PopovMP commented Jan 15, 2017

Yes, this make sense. However, I would recommend adding additional options to the standard find, findOne and count projection as: $skip, $limit, and $sort instead of adding new command.
Example:
db.find({}, {username: 1, email: 1, _id: 0, $skip: 20, $limit: 10, $sort: {username: 1} }, callback)

@iyobo
Copy link
Author

iyobo commented Jan 15, 2017

The main issue with that is neither count nor findOne are paged or sorted.
Secondarily, it's rather messy.

The criteria should remain seperate from query options.

@PopovMP
Copy link

PopovMP commented Jan 16, 2017

You may be right. However, I don't think we should add this pull request to the official distribution.
You may make a wrapper for NeDB with that functionality that will work over the original package.

I hope @louischatriot will take a clever decision.

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

Successfully merging this pull request may close these issues.

2 participants