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] Collection.find() & .aggregate() cursor generics #14012

Merged
merged 16 commits into from Feb 9, 2017
Merged

[mongodb] Collection.find() & .aggregate() cursor generics #14012

merged 16 commits into from Feb 9, 2017

Conversation

hinell
Copy link
Contributor

@hinell hinell commented Jan 13, 2017

Description

Featuring generics on the Cursor, AggregateCursor,
Collection.find() and Collection.aggregate() methods respectively.

Now we can do things like this:

interface USER{ _id: ObjectID; name: string;}
let users_col   = db.collection('col_name');
    users.find<USER>()        // Cursor returned
    .forEach(function(currentuser){
       console.log(user.name) // checking USER type
});

interface SUMMARY { id_: ObjectID,  total: number }
let payments = db.collection('payments');
    payments.aggregate<SUMMARY>() // AggregateCursor returned
    .toArray(function(err,sum){
      console.log(sum.total)
  })

Q: Why not to just declare the generic right on the db.collection() method and Collection interface as proposed by #13731?

Cause there are only a few methods that use the type that generic variable provides and also this would be sort of extra job creating mess and confusing users. So if you feel that you need another one extra generic-typed method like these one then just let me know.

Template

  • Make your PR against the master branch.
  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc without errors.
  • Run npm run lint package-name if a tslint.json is present.

@dt-bot
Copy link
Member

dt-bot commented Jan 13, 2017

mongodb/index.d.ts

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

Checklist

  • pass the Travis CI test?

@hinell
Copy link
Contributor Author

hinell commented Jan 13, 2017

Attention is needed guys. @CaselIT @beary

@CaselIT
Copy link
Contributor

CaselIT commented Jan 14, 2017

This will break backward compatibility.
As I mentioned #13731 (comment) I do not think there is a solution that maintains it until the issue microsoft/TypeScript#2175 is fixed.

If you want to use a typed version manually install it from the typings repo

typings i github:Think7/typings-mongodb#generics

@hinell
Copy link
Contributor Author

hinell commented Jan 14, 2017

@CaselIT

This will break backward compatibility.

You need to explain it more precisely and what the interface it will break exactly before considering pr to close.

@CaselIT
Copy link
Contributor

CaselIT commented Jan 14, 2017

If you used any of the classes that are now generic as a type annotation you will now have an error of the type error TS2314: Generic type '...' requires 1 type argument(s).

Example if you had:

let c: Cursor;

Typescript will not output error TS2314: Generic type 'Cursor' requires 1 type argument(s).

@hinell
Copy link
Contributor Author

hinell commented Jan 14, 2017

let c: Cursor;

Just use any type

let c: Cursor<any>;

Now NO backward compatibility is broken.

@CaselIT
Copy link
Contributor

CaselIT commented Jan 14, 2017

@hinell that is exactly the problem. If you have an existing project that uses the current definition that would break and you would need to go and modify all the types that are now generics.

@hinell
Copy link
Contributor Author

hinell commented Jan 14, 2017

@CaselIT
As you can see below travis show us no errros.
And meanwhile whoever need to use the Cursor type directly when it is already defined by collection.find<T>() method with T set to any by default?
Existing projects that depend on new interface won't have broke their compatibility if they use this method.

@CaselIT
Copy link
Contributor

CaselIT commented Jan 14, 2017

I'll let the maintainers decide.
I'm not for breaking existing code.

@hinell
Copy link
Contributor Author

hinell commented Jan 14, 2017

@CaselIT
It's fine but you have to show where the backward incompatibility is taking place before I consider this PR as useless.

Well guys what about if I'll extract generics into a separate interface?
upd: stupid idea

@bowdenk7
Copy link

bowdenk7 commented Feb 4, 2017

@hinell Looks like there are some merge conflicts. Please refresh the PR so we can merge.

@hinell
Copy link
Contributor Author

hinell commented Feb 4, 2017

@bowdenk7 Here we go...

@@ -1222,13 +1226,13 @@ export interface EndCallback {

//http://mongodb.github.io/node-mongodb-native/2.1/api/AggregationCursor.html#~resultCallback
export type AggregationCursorResult = any | void;

// TODO: change to generic version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove TODO.

// http://mongodb.github.io/node-mongodb-native/2.1/api/Cursor.html#clone
clone(): Cursor;
// https://github.com/christkv/mongodb-core/blob/2.0/lib/cursor.js#L333
clone(): Cursor<T>; // still returns the same type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove comment.

@aozgaa aozgaa merged commit ffb4f1f into DefinitelyTyped:master Feb 9, 2017
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.

None yet

6 participants