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

mongodb.d.ts fixes to enable Promise and async-await scenarios #9024

Closed
wants to merge 3 commits into from
Closed

Conversation

davetemplin
Copy link
Contributor

Various fixes to enable Promise and async-await scenarios...

  • Db.command: fix incorrect function overload signature
  • Collection.aggregate: fix incorrect function overload signature
  • Collection.find: add missing overload signature
  • Collection.findOne: add missing interface function
  • Collection.initializeOrderedBulkOp: correct type of option parameter
  • Collection.initializeUnorderedBulkOp: correct type of option parameter
  • Collection.insert: add missing interface function
  • Collection.update: add missing interface function
  • OrderedBulkOperation.execute: fix incorrect function overload signature
  • UnorderedBulkOperation.execute: fix incorrect function overload signature

Fix spelling errors: CollectionAggrigationOptions -> CollectionAggregationOptions

Various fixes to enable Promise and async-await scenarios...
  - Db.command: fix incorrect function overload signature
  - Collection.aggregate: fix incorrect function overload signature
  - Collection.find: add missing overload signature
  - Collection.findOne: add missing interface function
  - Collection.initializeOrderedBulkOp: correct type of option parameter
  - Collection.initializeUnorderedBulkOp: correct type of option parameter
  - Collection.insert: add missing interface function
  - Collection.update: add missing interface function
  - OrderedBulkOperation.execute: fix incorrect function overload signature
  - UnorderedBulkOperation.execute: fix incorrect function overload signature

Fix spelling errors: CollectionAggrigationOptions -> CollectionAggregationOptions
@dt-bot
Copy link
Member

dt-bot commented Apr 17, 2016

mongodb/mongodb.d.ts

to author (@CaselIT). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@CaselIT
Copy link
Contributor

CaselIT commented Apr 17, 2016

The options in initializeOrderedBulkOp and initializeUnorderedBulkOp were correct before according to the documentation here and here. I agree that they should be optional.
I've asked about the missing documentation for the other find options. I think we should limit to the documented signature.
insert, update and findOne where left out on purpose since they are deprecated.

@davetemplin
Copy link
Contributor Author

Totally understand the motive to omit legacy functions, but here's an argument to consider keeping them...

Omitting a legacy declaration prevents users from targeting a down-level (pre-3.2) mongodb instance that doesn't yet support the new interface. If the driver still exposes the interface (which it does), then so should the typing.

Arguably an ideal way to deal with this situation would be if the TypeScript compiler had a way to flag obsolete declarations. But since we don't have that feature yet, my suggestion would be to keep legacy declarations and perhaps comment them as //obsolete in the code.

@CaselIT
Copy link
Contributor

CaselIT commented Apr 17, 2016

Omitting a legacy declaration prevents users from targeting a down-level (pre-3.2) mongodb instance that doesn't yet support the new interface.

I guess you have a point here.

@CaselIT
Copy link
Contributor

CaselIT commented Apr 18, 2016

@davetemplin I've updated the definition with the deprecated method you suggested for typings. If you use it you can install them with typings i mongodb

@davetemplin
Copy link
Contributor Author

Thank you for making these updates. Great to see the legacy functions reintroduced but flagged as deprecated. However, a few other important details unfortunately got lost in the translation...

SUMMARY:

  1. For Collection.aggregate the return-type of "void|AggregationCursor" prevents applying cursor functions (such as toArray) without explicit casts.
  2. The secondary Collection.find overload was not added thus preventing projecting, skipping, limiting, and timeout functions.
  3. The Db.command function does not return a Promise if options is passed, which prevents accessing the result.
  4. The options argument on initializeOrderedBulkOp and initializeUnorderedBulkOp should be optional.

DETAILS:

1. For Collection.aggregate the return-type of "void|AggregationCursor" prevents applying cursor functions (such as toArray) without explicit casts.

For example we should be able to write a simple aggregate statement like the following...

EXAMPLE: var documents = await db.collection('test').aggregate([{$match:{name:"foo"}}]).toArray();
ERROR: Supplied parameters do not match any signature of call target. (options should be optional)
ERROR: Property 'toArray' does not exists on type 'void | AggregationCursor'. (return type should just be AggregationCursor instead)

Suggest in this case not trying to represent the void return type at all. Perhaps anyone who passes a callback isn't going to care about the return type anyway.

2. The secondary Collection.find overload was not added and prevents projecting, skipping and limiting.

EXAMPLE: var result = await db.collection('test').find({name:"foo"}, {_name:1}, 0, 10);
ERROR: Supplied parameters do not match any signature of call target. (project, skip, limit should be defined parameters)

3. The Db.command function does not return a Promise if options is passed, which prevents accessing the result.

EXAMPLE: var result = await db.command({ping:1});
ERROR: result is a void type but should be any type instead.

4. The options argument initializeOrderedBulkOp and initializeUnorderedBulkOp should be optional.

EXAMPLE: var bulk = db.collection('test').initializeUnorderedBulkOp();
ERROR: Supplied parameters do not match any signature of call target. (options should be optional)

SUGGESTED CHANGES:
Once again here are the suggested definitions to resolve all of the issues outlined above...

for Db ...

command(command: Object, callback: MongoCallback<any>): void;
command(command: Object, options?: { readPreference: ReadPreference | string }, callback?: MongoCallback<any>): Promise<any>;

Please note the callback must not be optional in the first declaration, otherwise it becomes ambiguous with the second declaration (masking out the promise since it is defined first). Again, suggest not worrying about the void case as above.

for Collection ...

aggregate(pipeline: Object[], options?: CollectionAggregationOptions, callback?: MongoCallback<any>): AggregationCursor;
find(query: Object, fields?: any, skip?: number, limit?: number, timeout?: number): Cursor;

for OrderedBulkOperation and UnorderedBulkOperation ...

execute(options?: FSyncOptions, callback?: MongoCallback<BulkWriteResult>): Promise<BulkWriteResult>;

I appreciate there are a lot of scenarios and subtleties here and it's really tough to validate them all. But these additional changes if made should make mongodb work really well with async-await!

@CaselIT
Copy link
Contributor

CaselIT commented Apr 19, 2016

@davetemplin I've added the updates you suggested, could you check them here Think7/typings-mongodb@deed273 ? Thanks for them.

Regarding the find method in the collection, the mongodb team suggested to use the cursor here. But maybe we could add the legacy signature with a deprecated annotation. What do you think?

@davetemplin
Copy link
Contributor Author

The changes on Think7 look good, thanks!! By the way I also just now noticed Collection.save is missing.

Regarding the find method I've observed performance issues on pre-3.0 instances where using cursor functions such as find({}).limit(1000) do not perform well when a theoretically equivalent legacy query like find({},null,null,1000) is instantaneous. So yes I think keeping the legacy signature with a deprecation warning as you suggest is definitely the way to go.

@CaselIT
Copy link
Contributor

CaselIT commented Apr 20, 2016

Added the missing signatures Think7/typings-mongodb@1061602
Should be available with typings as soon as the registry pull request is merged

@davetemplin
Copy link
Contributor Author

davetemplin commented Apr 20, 2016

Splendid, thanks!
Will the master copy on GitHub be updated as well?

...would like to see these changes have the broadest possible reach--so folks still using tsd or grabbing directly from GitHub directly will get these changes.

@CaselIT
Copy link
Contributor

CaselIT commented Apr 20, 2016

You could update this pull request with a new push, else I can do a new pr with the new updates.

@davetemplin
Copy link
Contributor Author

New pull request submitted. One last thing we didn't catch before was on Collection.aggregate the options and callback parameters were not marked as optional.

@CaselIT
Copy link
Contributor

CaselIT commented Apr 21, 2016

@davetemplin There is always something! 😄 I'll fix that.
I've commented the other pr. Thanks for the help :)

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.

3 participants