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

[Bug] TypeScript: Mismatch between documented return types and actual return types #3184

Open
andriadze opened this issue Nov 22, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@andriadze
Copy link

What happened?

There's a mismatch between what's documented in the comments and what's actually being returned:
For example here: https://github.com/chroma-core/chroma/blob/main/clients/js/src/Collection.ts

We can see that add, upsert and others return an empty Promise.
async add(params: AddRecordsParams): Promise<void>

But the comment right above that function says that it should return Promise of AddResponse:

/**
   * Add items to the collection
   * @param {Object} params - The parameters for the query.
   * @param {ID | IDs} [params.ids] - IDs of the items to add.
   * @param {Embedding | Embeddings} [params.embeddings] - Optional embeddings of the items to add.
   * @param {Metadata | Metadatas} [params.metadatas] - Optional metadata of the items to add.
   * @param {Document | Documents} [params.documents] - Optional documents of the items to add.
   * @returns {Promise<AddResponse>} - The response from the API. True if successful.
   *
   * @example
   * ```typescript
   * const response = await collection.add({
   *   ids: ["id1", "id2"],
   *   embeddings: [[1, 2, 3], [4, 5, 6]],
   *   metadatas: [{ "key": "value" }, { "key": "value" }],
   *   documents: ["document1", "document2"]
   * });
   * ```
   */
 async add(params: AddRecordsParams): Promise<void>

Versions

^1.9.2

Relevant log output

No response

@andriadze andriadze added the bug Something isn't working label Nov 22, 2024
@aramcheck
Copy link

aramcheck commented Dec 6, 2024

Hello! We bump into the same issue in one of the latest updates. Currently running with 1.9.4.

/**
 * Deletes items from the collection.
 * @param {Object} params - The parameters for deleting items from the collection.
 * @param {ID | IDs} [params.ids] - Optional ID or array of IDs of items to delete.
 * @param {Where} [params.where] - Optional query condition to filter items to delete based on metadata values.
 * @param {WhereDocument} [params.whereDocument] - Optional query condition to filter items to delete based on document content.
 * @returns {Promise<string[]>} A promise that resolves to the IDs of the deleted items.
 * @throws {Error} If there is an issue deleting items from the collection.
 *
 * @example
 * '''typescript
 * const results = await collection.delete({
 *   ids: "some_id",
 *   where: {"name": {"$eq": "John Doe"}},
 *   whereDocument: {"$contains":"search_string"}
 * });
 * '''
 */
delete({ ids, where, whereDocument, }?: DeleteParams): Promise<void>;

In our case we were wrapping the adapter with an interface that conformed with the types declared by Chroma. In the example above, the delete was annotated and returned Promise<string[]>. This change broke our implementation at type level.

To fix it we temporally did something like this:

async delete({ ids }: { ids: string[] }) {
  const collection = await this.getCollection();
  await collection.delete({ ids });
  return ids;
}

But the Chroma Collection definition is declaring different types to the ones documented making it hard to understand if the implementation was changed or is just a type annotation bug.

Among offending methods are:

  • add: Promise<void> vs Promise<AddResponse>
  • modify: Promise<CollectionParams> vs Promise<void>
  • peek: Promise<MultiGetResponse> vs Promise<GetResponse>
  • delete: Promise<void> vs Promise<string[]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants