Use ImportResponse[] as type of ImportError.importResults

[jasongwartz]

Change Summary

The docs indicate that the error thrown by import() (as in: client.collections(...).documents().import(...)) contains a key importResults. In my usage, I found that the runtime type of the value of importResults is an array, which seems to contain either ImportResponseSuccess or ImportResponseFail. At the moment I'm force-casting to override the type given by the library, but if my experimentation is correct, it'd be good to include the correct type here.

PR Checklist

• I have read and signed the Contributor License Agreement.

[sgarner]

I'm experiencing the same issue.

It's clear from the usage that an array of both successful and failed responses (resultsInJSONFormat) is being given as the second argument to the ImportError constructor:

typesense-js/src/Typesense/Documents.ts

Lines 412 to 417 in 7183419

	throw new ImportError(
	`${resultsInJSONFormat.length - failedItems.length
	} documents imported successfully, ${failedItems.length
	} documents failed during import. Use \`error.importResults\` from the raised exception to get a detailed error reason for each document.`,
	resultsInJSONFormat,
	);

And there is already a test which demonstrates that ImportError.importResults is intended to be an array:

typesense-js/test/Typesense/Documents.spec.js

Lines 523 to 526 in 7183419

	expect(error.constructor.name).to.eq("ImportError");
	expect(error.importResults.length).to.eq(2);
	expect(error.importResults[0].success).to.eq(false);
	expect(error.importResults[0].message).to.eq("Error message");

The fix for the types in this PR looks good, and I wonder why it hasn't been merged yet?

[sgarner]

I would just add that this mistake could have been picked up sooner if the ImportError class had typings on its constructor arguments. Unfortunately they're implicit any:

typesense-js/src/Typesense/Errors/ImportError.ts

Line 6 in 7183419

constructor(message, importResults) {

[tharropoulos]

I think a better way to approach this is, since the error is thrown, the most important information for the caller is which of the import statements failed. Currently, it will print all of the import responses. I think a better solution would be to use something like this:

import TypesenseError from "./TypesenseError";
import type { ImportResponseFail } from "../Documents";

export default class ImportError extends TypesenseError {
  importResults: ImportResponseFail[];
  constructor(message: string, importResults: ImportResponseFail[]) {
    super(message);
    this.importResults = importResults;
  }
}

And then, on the import function, handle errors like so:

    if (Array.isArray(documents)) {
      const resultsInJSONFormat = resultsInJSONLFormat
        .split("\n")
        .map((r) => JSON.parse(r)) as ImportResponse[];
      const failedItems = resultsInJSONFormat.filter(
        (r) => r.success === false,
      ) as ImportResponseFail[];
      if (failedItems.length > 0) {
        throw new ImportError(
          `${
            resultsInJSONFormat.length - failedItems.length
          } documents imported successfully, ${
            failedItems.length
          } documents failed during import. Use \`error.importResults\` from the raised exception to get a detailed error reason for each document.`,
          failedItems,
        );
      } else {...

One thing also worth noting is to later introduce zod to safely parse JSON to the desired type like documented here. This is out of the scope of this PR and something that should be introduced separately.

[tharropoulos]

We should add type annotations to the constructor as well, as @sgarner mentioned, as even with this change, the Typescript compiler will still issue an error for implicit any's, if set to strict.

export default class ImportError extends TypesenseError {
  importResults: ImportResponse[];
  constructor(message: string, importResults: ImportResponse[]) {...

[sgarner]

Thank you both for following up on this 👍

I think a better way to approach this is, since the error is thrown, the most important information for the caller is which of the import statements failed. Currently, it will print all of the import responses.

If the thrown error only contains the failed responses, the calling application will have no way to inspect the successful responses. So I would favour retaining the current behaviour. (This would also be a breaking change.)

[tharropoulos]

Thank you both for following up on this 👍

I think a better way to approach this is, since the error is thrown, the most important information for the caller is which of the import statements failed. Currently, it will print all of the import responses.

If the thrown error only contains the failed responses, the calling application will have no way to inspect the successful responses. So I would favour retaining the current behaviour. (This would also be a breaking change.)

Another way to approach this is leaving it to the caller to unwrap it themselves, although that breaks even more things in terms of backwards compatibility. So I think we should leave it as is for the time being then! As soon as @jasongwartz has the type annotations in place, I think we're done

[jasonbosco]

Thank you for the PR!

[jasonbosco]

Published in 2.0.0-1