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

fix(NODE-3468): remove generic overrides from find #2935

Merged
merged 12 commits into from
Aug 24, 2021

Conversation

nbbeeken
Copy link
Contributor

Removing the generics from find* and toArray/next will make the result type more logical in scenarios where there is not schema or the filter specified is an incorrect type.

Removed the same behavior from listCollections
Added a generic to db.aggregate where it should have been

Q: do we want to do something better with specifying a projection in your options argument to find* right now you can only "fix" the result with the .project<>() builder style, maybe we continue to accept generics on find*. but we no longer make it Filter<T>, the filter still has to be based on the schema (Filter<TSchema>) but the result can be changed to the specified type in the generic. thoughts?

Copy link
Contributor

@PaulEndri PaulEndri left a comment

Choose a reason for hiding this comment

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

Regarding your question re: the .find functionality: I personally would find it super helpful. Since the result of the .find can actually vary significantly, it makes sense to be able to type the final output while making sure the filter follows the schema. Just my .02

next<T = TSchema>(callback: Callback<T | null>): void;
next<T = TSchema>(callback?: Callback<T | null>): Promise<T | null> | void {
next(): Promise<TSchema | null>;
next(callback: Callback<TSchema | null>): void;

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our next functions explicitly return null when the end of the cursor is reached. Our callback definition only lets the second are be potentially undefined, so the | null is very technically necessary, if one is triple equals checking for null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. The following is purely pedantry and feel free to ignore it, because I likely would if I were you lol.

For transparency, do think a helper type here would be useful? So instead of showing the | null it would be Promise<EndOfCursor<TSchema>> where EndOfCursor is defined as T | null? Again, super minute and what not. Marking this comment as resolved so it's out of mind lol. Thanks for answering my questions!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true we have this repeated in a number of places, consolidating it to type would help us make sure we don't get one override different from the others, but I'll defer that to an enhancement effort down the line for now I want this PR to unblock the issue with the generics, but I do appreciate the suggestion! thanks!

@@ -413,7 +412,7 @@ export abstract class AbstractCursor<
// these do need to be transformed since they are copying the rest of the batch
const internalDocs = (transform
? this[kDocuments].splice(0, this[kDocuments].length).map(transform)
: this[kDocuments].splice(0, this[kDocuments].length)) as T[]; // TODO(NODE-3283): Improve transform typing
: this[kDocuments].splice(0, this[kDocuments].length)) as TSchema[]; // TODO(NODE-3283): Improve transform typing
Copy link
Contributor

Choose a reason for hiding this comment

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

can this[kDocuments] be typed as TSchema[] ? In the class definition itself so these as TSchema[] hacks can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the type of this[kDocuments] although you did make me think of a potential improvement (really just internally for us) but if we instead type this[kTransform] to (doc: TSchema) => TSchema then we don't need these casts.

When you pass the transform into .map() it will be (doc: TSchema) => T but map will return a new Cursor<T>. Where T is from the result type of the function passed to .map. So the argument that goes into this[kTransform] is the old schema and it returns the "current" one.

I suppose the most accurate would be this[kTransform]: (doc: unknown) => TSchema but then we need equivalent casting/whatever to narrow unknown

Copy link
Contributor

Choose a reason for hiding this comment

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

Well my original comment was because I didn't see a type on kDocument and was thinking about ways to get rid of the as because this[kDocuments].splice should automatically return something with the type of <Type of<this[kDocuments]>>[] so in this case if the source array was already typed, then this would automatically be TSchema[] so the typing here made me curious.

I actually played around a lot with prototyping a change to ktransform and others when thinking about ways to be able to type projections fully, and narrowing that down was like, a several hundred line TS object that existed only for being able to pass around the type accurately, and I think the unknown above might run into the same situation? Although would you be able to use the Partial keyword here for some help? I like the idea of the different typing, just wondering if the effort required to type that unknown is worth it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the lack of reply, been focused on getting this PR ready for fixing the user issues. I think accurately typing the transform is something we should look into, but it would only affect our internal TS code so at the moment we wouldn't need to prioritize it. but going forward more accuracy even if it requires TS helpers etc. could help catch any issues here.

Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

I need to review more of this, but I wanted to capture at least some feedback

test/types/list_collections.test-d.ts Outdated Show resolved Hide resolved
test/types/list_collections.test-d.ts Outdated Show resolved Hide resolved
test/types/list_collections.test-d.ts Outdated Show resolved Hide resolved
test/types/list_collections.test-d.ts Outdated Show resolved Hide resolved
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Aug 17, 2021
@dariakp dariakp self-assigned this Aug 17, 2021
test/types/community/cursor.test-d.ts Outdated Show resolved Hide resolved
test/types/community/cursor.test-d.ts Outdated Show resolved Hide resolved
test/types/community/collection/findX.test-d.ts Outdated Show resolved Hide resolved
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Aug 24, 2021
@dariakp dariakp requested a review from emadum August 24, 2021 14:05
@nbbeeken nbbeeken requested a review from dariakp August 24, 2021 15:59
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

@dariakp dariakp merged commit 74bd7bd into 4.1 Aug 24, 2021
@dariakp dariakp deleted the NODE-3468/fix-auto-generic branch August 24, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants