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.

Sign up for GitHub

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

Jump to bottom

types(NODE-3729): add id type to TSchema return types #3020

Closed
wants to merge 1 commit into from
Closed

types(NODE-3729): add id type to TSchema return types #3020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
},
"scripts": {
"build:evergreen": "node .evergreen/generate_evergreen_tasks.js",
"build:ts": "rimraf lib && ./node_modules/typescript/bin/tsc",
"build:ts": "rimraf lib && node ./node_modules/typescript/bin/tsc",
"build:dts": "npm run build:ts && api-extractor run && rimraf 'lib/**/*.d.ts*' && downlevel-dts mongodb.d.ts mongodb.ts34.d.ts",
"build:docs": "typedoc",
"check:bench": "node test/benchmarks/driverBench",
Expand Down
37 changes: 19 additions & 18 deletions src/collection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,13 @@ import type {
UpdateFilter,
WithId,
OptionalId,
Flatten
Flatten,
InferIdType
} from './mongo_types';

/** @public */
export interface ModifyResult<TSchema = Document> {
value: TSchema | null;
value: WithId<TSchema> | null;
lastErrorObject?: Document;
ok: 0 | 1;
}
Expand Down Expand Up @@ -676,37 +677,37 @@ export class Collection<TSchema extends Document = Document> {
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
findOne(): Promise<TSchema | null>;
findOne(callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, options: FindOptions, callback: Callback<TSchema | null>): void;
findOne(): Promise<WithId<TSchema> | null>;
findOne(callback: Callback<WithId<TSchema> | null>): void;
findOne(filter: Filter<TSchema>): Promise<WithId<TSchema> | null>;
findOne(filter: Filter<TSchema>, callback: Callback<WithId<TSchema> | null>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<WithId<TSchema> | null>;
findOne(filter: Filter<TSchema>, options: FindOptions, callback: Callback<WithId<TSchema> | null>): void;

// allow an override of the schema.
findOne<T = TSchema>(): Promise<T | null>;
findOne<T = TSchema>(callback: Callback<T | null>): void;
findOne<T = TSchema>(filter: Filter<TSchema>): Promise<T | null>;
findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions): Promise<T | null>;
findOne<T = TSchema>(): Promise<WithId<T> | null>;
findOne<T = TSchema>(callback: Callback<WithId<T> | null>): void;
findOne<T = TSchema>(filter: Filter<TSchema>): Promise<WithId<T> | null>;
findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions): Promise<WithId<T> | null>;
Comment on lines +688 to +691
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of these overrides is to allow users to specify an exact return type (to account for projections that modify the shape of documents returned) So we can't enforce WithId on the result type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can create a new overload that accounts for projection with no _id, but I believe the other ones should stay.
Would be something like this

  findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions & { projection: { _id: 0 } }): Promise<T | null>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This overload actually creates way more issues as it doesn't match with the general function declaration, can we go without it?

findOne<T = TSchema>(
filter: Filter<TSchema>,
options?: FindOptions,
callback?: Callback<T | null>
callback?: Callback<WithId<T> | null>
): void;

findOne(
filter?: Filter<TSchema> | Callback<TSchema | null>,
options?: FindOptions | Callback<TSchema | null>,
callback?: Callback<TSchema | null>
): Promise<TSchema | null> | void {
filter?: Filter<TSchema> | Callback<WithId<TSchema> | null>,
options?: FindOptions | Callback<WithId<TSchema> | null>,
callback?: Callback<WithId<TSchema> | null>
): Promise<TSchema & InferIdType<TSchema> | null> | void {
if (callback != null && typeof callback !== 'function') {
throw new MongoInvalidArgumentError(
'Third parameter to `findOne()` must be a callback or undefined'
);
}

if (typeof filter === 'function') {
callback = filter as Callback<TSchema | null>;
callback = filter as Callback<WithId<TSchema> | null>;
filter = {};
options = {};
}
Expand Down
52 changes: 26 additions & 26 deletions src/cursor/abstract_cursor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type { Topology } from '../sdam/topology';
import { Readable, Transform } from 'stream';
import type { ExecutionResult } from '../operations/execute_operation';
import { ReadConcern, ReadConcernLike } from '../read_concern';
import { TODO_NODE_3286, TypedEventEmitter } from '../mongo_types';
import { TODO_NODE_3286, TypedEventEmitter, WithId } from '../mongo_types';

/** @internal */
const kId = Symbol('id');
Expand Down Expand Up @@ -112,11 +112,11 @@ export abstract class AbstractCursor<
/** @internal */
[kNamespace]: MongoDBNamespace;
/** @internal */
[kDocuments]: TSchema[];
[kDocuments]: WithId<TSchema>[];
Copy link
Contributor

Choose a reason for hiding this comment

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

What type issue is requiring the cursor class to be modified? I think nothing here should have to change.
I feel like where ever we create cursors, the WithId modifier would pass through the class's parameterization. Using FindCursor<WithId<TSchema>> or FindCursor<TSchema> would make all the APIs the cursor has return the correct type, I believe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in some functions like on line 235. Reverting the change yields this error:
image

/** @internal */
[kTopology]: Topology;
/** @internal */
[kTransform]?: (doc: TSchema) => Document;
[kTransform]?: (doc: WithId<TSchema>) => Document;
/** @internal */
[kInitialized]: boolean;
/** @internal */
Expand Down Expand Up @@ -231,11 +231,11 @@ export abstract class AbstractCursor<
}

/** Returns current buffered documents */
readBufferedDocuments(number?: number): TSchema[] {
readBufferedDocuments(number?: number): WithId<TSchema>[] {
return this[kDocuments].splice(0, number ?? this[kDocuments].length);
}

[Symbol.asyncIterator](): AsyncIterator<TSchema, void> {
[Symbol.asyncIterator](): AsyncIterator<WithId<TSchema>, void> {
return {
next: () =>
this.next().then(value =>
Expand Down Expand Up @@ -280,7 +280,7 @@ export abstract class AbstractCursor<
return done(undefined, true);
}

next<TSchema>(this, true, (err, doc) => {
next<WithId<TSchema>>(this, true, (err, doc) => {
if (err) return done(err);

if (doc) {
Expand All @@ -295,10 +295,10 @@ export abstract class AbstractCursor<
}

/** Get the next available document from the cursor, returns null if no more documents are available. */
next(): Promise<TSchema | null>;
next(callback: Callback<TSchema | null>): void;
next(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void;
next(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void {
next(): Promise<WithId<TSchema> | null>;
next(callback: Callback<WithId<TSchema> | null>): void;
next(callback?: Callback<WithId<TSchema> | null>): Promise<WithId<TSchema> | null> | void;
next(callback?: Callback<WithId<TSchema> | null>): Promise<WithId<TSchema> | null> | void {
return maybePromise(callback, done => {
if (this[kId] === Long.ZERO) {
return done(new MongoCursorExhaustedError());
Expand All @@ -311,9 +311,9 @@ export abstract class AbstractCursor<
/**
* Try to get the next available document from the cursor or `null` if an empty batch is returned
*/
tryNext(): Promise<TSchema | null>;
tryNext(callback: Callback<TSchema | null>): void;
tryNext(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void {
tryNext(): Promise<WithId<TSchema> | null>;
tryNext(callback: Callback<WithId<TSchema> | null>): void;
tryNext(callback?: Callback<WithId<TSchema> | null>): Promise<WithId<TSchema> | null> | void {
return maybePromise(callback, done => {
if (this[kId] === Long.ZERO) {
return done(new MongoCursorExhaustedError());
Expand All @@ -329,10 +329,10 @@ export abstract class AbstractCursor<
* @param iterator - The iteration callback.
* @param callback - The end callback.
*/
forEach(iterator: (doc: TSchema) => boolean | void): Promise<void>;
forEach(iterator: (doc: TSchema) => boolean | void, callback: Callback<void>): void;
forEach(iterator: (doc: WithId<TSchema>) => boolean | void): Promise<void>;
forEach(iterator: (doc: WithId<TSchema>) => boolean | void, callback: Callback<void>): void;
forEach(
iterator: (doc: TSchema) => boolean | void,
iterator: (doc: WithId<TSchema>) => boolean | void,
callback?: Callback<void>
): Promise<void> | void {
if (typeof iterator !== 'function') {
Expand All @@ -341,7 +341,7 @@ export abstract class AbstractCursor<
return maybePromise(callback, done => {
const transform = this[kTransform];
const fetchDocs = () => {
next<TSchema>(this, true, (err, doc) => {
next<WithId<TSchema>>(this, true, (err, doc) => {
if (err || doc == null) return done(err);
let result;
// NOTE: no need to transform because `next` will do this automatically
Expand All @@ -358,7 +358,7 @@ export abstract class AbstractCursor<
for (let i = 0; i < internalDocs.length; ++i) {
try {
result = iterator(
(transform ? transform(internalDocs[i]) : internalDocs[i]) as TSchema // TODO(NODE-3283): Improve transform typing
(transform ? transform(internalDocs[i]) : internalDocs[i]) as WithId<TSchema> // TODO(NODE-3283): Improve transform typing
);
} catch (error) {
return done(error);
Expand Down Expand Up @@ -402,15 +402,15 @@ export abstract class AbstractCursor<
*
* @param callback - The result callback.
*/
toArray(): Promise<TSchema[]>;
toArray(callback: Callback<TSchema[]>): void;
toArray(callback?: Callback<TSchema[]>): Promise<TSchema[]> | void {
toArray(): Promise<WithId<TSchema>[]>;
toArray(callback: Callback<WithId<TSchema>[]>): void;
toArray(callback?: Callback<WithId<TSchema>[]>): Promise<WithId<TSchema>[]> | void {
return maybePromise(callback, done => {
const docs: TSchema[] = [];
const docs: WithId<TSchema>[] = [];
const transform = this[kTransform];
const fetchDocs = () => {
// NOTE: if we add a `nextBatch` then we should use it here
next<TSchema>(this, true, (err, doc) => {
next<WithId<TSchema>>(this, true, (err, doc) => {
if (err) return done(err);
if (doc == null) return done(undefined, docs);

Expand All @@ -422,7 +422,7 @@ export abstract class AbstractCursor<
transform
? this[kDocuments].splice(0, this[kDocuments].length).map(transform)
: this[kDocuments].splice(0, this[kDocuments].length)
) as TSchema[]; // TODO(NODE-3283): Improve transform typing
) as WithId<TSchema>[]; // TODO(NODE-3283): Improve transform typing

if (internalDocs) {
docs.push(...internalDocs);
Expand Down Expand Up @@ -475,9 +475,9 @@ export abstract class AbstractCursor<
* ```
* @param transform - The mapping transformation method.
*/
map<T = any>(transform: (doc: TSchema) => T): AbstractCursor<T> {
map<T = any>(transform: (doc: WithId<TSchema>) => T): AbstractCursor<T> {
assertUninitialized(this);
const oldTransform = this[kTransform] as (doc: TSchema) => TSchema; // TODO(NODE-3283): Improve transform typing
const oldTransform = this[kTransform] as (doc: WithId<TSchema>) => WithId<TSchema>; // TODO(NODE-3283): Improve transform typing
if (oldTransform) {
this[kTransform] = doc => {
return transform(oldTransform(doc));
Expand Down
3 changes: 2 additions & 1 deletion src/cursor/find_cursor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type { ClientSession } from '../sessions';
import { formatSort, Sort, SortDirection } from '../sort';
import type { Callback, MongoDBNamespace } from '../utils';
import { AbstractCursor, assertUninitialized } from './abstract_cursor';
import type { WithId } from "../mongo_types"

/** @internal */
const kFilter = Symbol('filter');
Expand Down Expand Up @@ -64,7 +65,7 @@ export class FindCursor<TSchema = Document> extends AbstractCursor<TSchema> {
});
}

map<T>(transform: (doc: TSchema) => T): FindCursor<T> {
map<T>(transform: (doc: WithId<TSchema>) => T): FindCursor<T> {
return super.map(transform) as FindCursor<T>;
}

Expand Down
3 changes: 2 additions & 1 deletion src/operations/map_reduce.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type { ObjectId } from '../bson';
import { Aspect, defineAspects } from './operation';
import type { ClientSession } from '../sessions';
import { Db } from '../db';
import type { WithId } from "../mongo_types"

const exclusionList = [
'explain',
Expand All @@ -37,7 +38,7 @@ const exclusionList = [
];

/** @public */
export type MapFunction<TSchema = Document> = (this: TSchema) => void;
export type MapFunction<TSchema = Document> = (this: WithId<TSchema>) => void;
/** @public */
export type ReduceFunction<TKey = ObjectId, TValue = any> = (key: TKey, values: TValue[]) => TValue;
/** @public */
Expand Down
6 changes: 3 additions & 3 deletions test/types/mongodb.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Collection } from '../../src/collection';
import { AggregationCursor } from '../../src/cursor/aggregation_cursor';
import type { FindCursor } from '../../src/cursor/find_cursor';
import type { Document } from 'bson';
import { Db } from '../../src';
import { Db, WithId } from '../../src';
import { Topology } from '../../src/sdam/topology';
import * as MongoDBDriver from '../../src';

Expand All @@ -23,13 +23,13 @@ expectNotDeprecated(MongoDBDriver.ObjectId);
const client = new MongoClient('');
const coll = client.db('test').collection('test');
const findCursor = coll.find();
expectType<Document | null>(await findCursor.next());
expectType<WithId<Document> | null>(await findCursor.next());
const mappedFind = findCursor.map<number>(obj => Object.keys(obj).length);
expectType<FindCursor<number>>(mappedFind);
expectType<number | null>(await mappedFind.next());
expectType<number[]>(await mappedFind.toArray());
const aggCursor = coll.aggregate();
expectType<Document | null>(await aggCursor.next());
expectType<WithId<Document> | null>(await aggCursor.next());
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to consider more testing. If you look in test/types/community/collection/findX.test-d.ts there's some example schemas making use of the find APIs. And test/types/community/cursor.test-d.ts has cursor API tests.

const mappedAgg = aggCursor.map<number>(obj => Object.keys(obj).length);
expectType<AggregationCursor<number>>(mappedAgg);
expectType<number | null>(await mappedAgg.next());
Expand Down