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-6029): update types for collection listing indexes #4072

Merged

Conversation

prenaissance
Copy link
Contributor

@prenaissance prenaissance commented Apr 8, 2024

Description

What is changing?

Update the return type of Collection.prototype.indexes and Collection.prototype.indexInformation to an array of indexes with full properties when the full option is true and to a Record of <index name> - <keys and direction> when the full option is false.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

The old types were incorrect in regards to the full option and were very wide. This will avoid making some type mistakes when using these methods.

Release Highlight

Index information helpers have accurate Typescript return types

Collection.indexInformation(), Collection.indexes() and Db.indexInformation() are helpers that return index information for a given collection or database. These helpers take an option, full, that configures whether the return value contains full index descriptions or a compact summary:

collection.indexes({ full: true });   // returns an array of index descriptions
collection.indexes({ full: false });  // returns an object, mapping index names to index keys

However, the Typescript return type of these helpers was always Document. Thanks to @prenaissance, these helpers now have accurate type information! The helpers return a new type, IndexDescriptionCompact | IndexDescriptionInfo[], which accurately reflects the return type of these helpers. The helpers also support type narrowing by providing a boolean literal as an option to the API:

collection.indexes();   // returns `IndexDescriptionCompact | IndexDescriptionInfo[]`
collection.indexes({ full: false });  // returns an `IndexDescriptionCompact`
collection.indexes({ full: true });  // returns an `IndexDescriptionInfo[]`

collection.indexInfo();   // returns `IndexDescriptionCompact | IndexDescriptionInfo[]`
collection.indexInfo({ full: false });  // returns an `IndexDescriptionCompact`
collection.indexInfo({ full: true });  // returns an `IndexDescriptionInfo[]`

db.indexInfo();   // returns `IndexDescriptionCompact | IndexDescriptionInfo[]`
db.indexInfo({ full: false });  // returns an `IndexDescriptionCompact`
db.indexInfo({ full: true });  // returns an `IndexDescriptionInfo[]`

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@prenaissance prenaissance changed the title fix(NODE-6029): update types for collection listing collection indexes fix(NODE-6029): update types for collection listing indexes Apr 9, 2024
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Hey @prenaissance - thanks for the submission! I left some comments about the implementation.

Comment on lines 221 to 225
export type IndexDescriptionInfo = Omit<IndexDescription, 'key' | 'version'> & {
key: { [key: string]: IndexDirection };
v?: IndexDescription['version'];
};

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally try to keep our type definitions for server responses flexible so that if the server modifies a response in a future server version the driver is still compatible with it. Could we & Document here? I think that will provide type safety on known keys but will allow unknown keys without a Typescript error.

@@ -693,8 +701,13 @@ export class Collection<TSchema extends Document = Document> {
*
* @param options - Optional settings for the command
*/
async indexInformation(options?: IndexInformationOptions): Promise<Document> {
return await this.indexes({ ...options, full: options?.full ?? false });
async indexInformation<TFull extends boolean = false>(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's preferrable to avoid computed types as the return types from our APIs, since they don't provided users with information about the return type of a function until a user 1) reads the source of the computed type or 2) writes code using the API and checks the computed return type. Could we accomplish the same here using overloads and removing all TFull generics? Something like:

  indexInformation(
    options: IndexInformationOptions & { full: true }
  ): Promise<IndexDescriptionInfo[]>;
  indexInformation(
    options: IndexInformationOptions & { full: false }
  ): Promise<IndexDescriptionCompact>;
  indexInformation(
    options: IndexInformationOptions & { full: boolean }
  ): Promise<IndexDescriptionCompact | IndexDescriptionInfo[]>;

This has the additional benefits of

  • better documentation, because our tooling will generate a doc entry for each overload
  • clearly defined overloads with existing IDE tooling
  • each overload can be documented originally (if desired).

for example - here's what VSCode could hypothetically display if we used overloads:

Screenshot 2024-04-09 at 12 23 01 PM Screenshot 2024-04-09 at 12 22 55 PM

@@ -72,7 +72,7 @@ export type IndexSpecification = OneOrMore<
>;

/** @public */
export interface IndexInformationOptions extends ListIndexesOptions {
export interface IndexInformationOptions<TFull extends boolean> extends ListIndexesOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider this a breaking change, since existing users' code that makes use of IndexInformationOptions would break because they wouldn't be providing a generic argument. We could fix this by providing a default, but I think overloads are preferable (see my comment on Collection.indexInformation()).

@prenaissance
Copy link
Contributor Author

Hey @prenaissance - thanks for the submission! I left some comments about the implementation.

Thank you for the feedback. I fixed the comments and also added tests for Db.prototype.indexInformation

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

one last round of small comments, otherwise LGTM

src/collection.ts Outdated Show resolved Hide resolved
src/collection.ts Outdated Show resolved Hide resolved
src/collection.ts Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
@baileympearson baileympearson added the Team Review Needs review from team label Apr 11, 2024
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

I just updated the description from Document.prototype call outs to Collection.prototype

@baileympearson baileympearson merged commit 232bf3c into mongodb:main Apr 11, 2024
16 of 26 checks passed
baileympearson pushed a commit to baileympearson/node-mongodb-native that referenced this pull request Apr 15, 2024
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.

3 participants