Merge #1656

1656: Improve errors r=curquiza a=flevi29 # Pull Request ## Related issues Fixes #1612, #1655 ## What does this PR do? This PR aims to improve errors, so that they can contain all the necessary information, and more. - Prevent browser test `jsdom` from replacing `fetch` and `AbortController` for consistency with node tests, replacing previous solution where we removed the builtin `fetch` from node tests - Remove `"abort-controller"` package, it was only used in tests and now `AbortController` is always available - Rename `MeiliSearchCommunicationError` to `MeiliSearchRequestError`, as this error might not be entirely related to communication, but rather the request itself - Make errors use [`cause`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause), preserving the original error, simplifying things, taking advantage of modern browsers and runtimes actually printing this property - Remove the use of `Object.setPrototypeOf` in errors, this is not needed in modern browsers, and bundlers take care of it if we need to support older browsers (so in UMD bundle it's done twice currently). (https://stackoverflow.com/a/76851585) - Remove the use of `Error.captureStackTrace`, this is done by the base `Error` constructor, and it's only available in V8 engine based browsers/runtimes - https://v8.dev/docs/stack-trace-api - https://nodejs.org/api/errors.html#new-errormessage-options - https://stackoverflow.com/a/64063868 - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack - Only catch the error from `fetch` to re-throw it as `MeiliSearchRequestError`, other potential errors should propagate as they're either truly unexpected or are thrown by us, simplifying error handling and not putting unexpected errors under the `MeiliSearchError` umbrella - Rename `MeiliSearchErrorInfo` type to `MeiliSearchErrorResponse` - Other minor changes/improvements NOTE: Tests are horrifying, I didn't change all that much in src, but I had to change almost every test and by quite a bit. Testing is what I should aim to improve ASAP. Co-authored-by: F. Levi <[email protected]> Co-authored-by: Bruno Casali <[email protected]>
meilisearch · Jul 24, 2024 · 71dcebe · 71dcebe
2 parents 01f51b4 + 4b3dc78
commit 71dcebe
Show file tree

Hide file tree

Showing 45 changed files with 437 additions and 793 deletions.
diff --git a/jest-disable-built-in-fetch.js b/jest-disable-built-in-fetch.js
diff --git a/jest.config.js b/jest.config.js
@@ -16,7 +16,6 @@ const config = {
     'jest-watch-typeahead/filename',
     'jest-watch-typeahead/testname',
   ],
-  globalSetup: './jest-disable-built-in-fetch.js',
   projects: [
     {
       preset: 'ts-jest',
@@ -28,6 +27,8 @@ const config = {
         'env/',
         'token.test.ts',
       ],
+      // make sure built-in Node.js fetch doesn't get replaced for consistency
+      globals: { fetch: global.fetch, AbortController: global.AbortController },
     },
     {
       preset: 'ts-jest',

diff --git a/package.json b/package.json
@@ -84,7 +84,6 @@
     "@types/jest": "^29.5.11",
     "@typescript-eslint/eslint-plugin": "^6.19.0",
     "@typescript-eslint/parser": "^6.19.0",
-    "abort-controller": "^3.0.0",
     "brotli-size": "^4.0.0",
     "eslint": "^8.56.0",
     "eslint-config-prettier": "^9.1.0",

diff --git a/src/errors/http-error-handler.ts b/src/errors/http-error-handler.ts
diff --git a/src/errors/index.ts b/src/errors/index.ts
@@ -1,6 +1,5 @@
-export * from './http-error-handler';
 export * from './meilisearch-api-error';
-export * from './meilisearch-communication-error';
+export * from './meilisearch-request-error';
 export * from './meilisearch-error';
 export * from './meilisearch-timeout-error';
 export * from './version-hint-message';
diff --git a/src/errors/meilisearch-api-error.ts b/src/errors/meilisearch-api-error.ts
@@ -1,30 +1,20 @@
-import { MeiliSearchErrorInfo } from '../types';
+import { MeiliSearchErrorResponse } from '../types';
 import { MeiliSearchError } from './meilisearch-error';
 
-const MeiliSearchApiError = class extends MeiliSearchError {
-  httpStatus: number;
-  code: string;
-  link: string;
-  type: string;
-  stack?: string;
+export class MeiliSearchApiError extends MeiliSearchError {
+  override name = 'MeiliSearchApiError';
+  override cause?: MeiliSearchErrorResponse;
+  readonly response: Response;
 
-  constructor(error: MeiliSearchErrorInfo, status: number) {
-    super(error.message);
+  constructor(response: Response, responseBody?: MeiliSearchErrorResponse) {
+    super(
+      responseBody?.message ?? `${response.status}: ${response.statusText}`,
+    );
 
-    // Make errors comparison possible. ex: error instanceof MeiliSearchApiError.
-    Object.setPrototypeOf(this, MeiliSearchApiError.prototype);
+    this.response = response;
 
-    this.name = 'MeiliSearchApiError';
-
-    this.code = error.code;
-    this.type = error.type;
-    this.link = error.link;
-    this.message = error.message;
-    this.httpStatus = status;
-
-    if (Error.captureStackTrace) {
-      Error.captureStackTrace(this, MeiliSearchApiError);
+    if (responseBody !== undefined) {
+      this.cause = responseBody;
     }
   }
-};
-export { MeiliSearchApiError };
+}
diff --git a/src/errors/meilisearch-communication-error.ts b/src/errors/meilisearch-communication-error.ts
diff --git a/src/errors/meilisearch-error.ts b/src/errors/meilisearch-error.ts
@@ -1,16 +1,7 @@
-class MeiliSearchError extends Error {
-  constructor(message: string) {
-    super(message);
+export class MeiliSearchError extends Error {
+  override name = 'MeiliSearchError';
 
-    // Make errors comparison possible. ex: error instanceof MeiliSearchError.
-    Object.setPrototypeOf(this, MeiliSearchError.prototype);
-
-    this.name = 'MeiliSearchError';
-
-    if (Error.captureStackTrace) {
-      Error.captureStackTrace(this, MeiliSearchError);
-    }
+  constructor(...params: ConstructorParameters<typeof Error>) {
+    super(...params);
   }
 }
-
-export { MeiliSearchError };
diff --git a/src/errors/meilisearch-request-error.ts b/src/errors/meilisearch-request-error.ts
@@ -0,0 +1,9 @@
+import { MeiliSearchError } from './meilisearch-error';
+
+export class MeiliSearchRequestError extends MeiliSearchError {
+  override name = 'MeiliSearchRequestError';
+
+  constructor(url: string, cause: unknown) {
+    super(`Request to ${url} has failed`, { cause });
+  }
+}
diff --git a/src/errors/meilisearch-timeout-error.ts b/src/errors/meilisearch-timeout-error.ts
@@ -1,18 +1,9 @@
 import { MeiliSearchError } from './meilisearch-error';
 
-class MeiliSearchTimeOutError extends MeiliSearchError {
+export class MeiliSearchTimeOutError extends MeiliSearchError {
+  override name = 'MeiliSearchTimeOutError';
+
   constructor(message: string) {
     super(message);
-
-    // Make errors comparison possible. ex: error instanceof MeiliSearchTimeOutError.
-    Object.setPrototypeOf(this, MeiliSearchTimeOutError.prototype);
-
-    this.name = 'MeiliSearchTimeOutError';
-
-    if (Error.captureStackTrace) {
-      Error.captureStackTrace(this, MeiliSearchTimeOutError);
-    }
   }
 }
-
-export { MeiliSearchTimeOutError };
diff --git a/src/http-requests.ts b/src/http-requests.ts
@@ -3,8 +3,8 @@ import { PACKAGE_VERSION } from './package-version';
 
 import {
   MeiliSearchError,
-  httpResponseErrorHandler,
-  httpErrorHandler,
+  MeiliSearchApiError,
+  MeiliSearchRequestError,
 } from './errors';
 
 import { addTrailingSlash, addProtocolIfNotPresent } from './utils';
@@ -143,35 +143,36 @@ class HttpRequests {
     }
 
     const headers = { ...this.headers, ...config.headers };
+    const responsePromise = this.fetchWithTimeout(
+      constructURL.toString(),
+      {
+        ...config,
+        ...this.requestConfig,
+        method,
+        body,
+        headers,
+      },
+      this.requestTimeout,
+    );
 
-    try {
-      const result = this.fetchWithTimeout(
-        constructURL.toString(),
-        {
-          ...config,
-          ...this.requestConfig,
-          method,
-          body,
-          headers,
-        },
-        this.requestTimeout,
-      );
-
-      // When using a custom HTTP client, the response is returned to allow the user to parse/handle it as they see fit
-      if (this.httpClient) {
-        return await result;
-      }
+    const response = await responsePromise.catch((error: unknown) => {
+      throw new MeiliSearchRequestError(constructURL.toString(), error);
+    });
+
+    // When using a custom HTTP client, the response is returned to allow the user to parse/handle it as they see fit
+    if (this.httpClient !== undefined) {
+      return response;
+    }
 
-      const response = await result.then((res: any) =>
-        httpResponseErrorHandler(res),
-      );
-      const parsedBody = await response.json().catch(() => undefined);
+    const responseBody = await response.text();
+    const parsedResponse =
+      responseBody === '' ? undefined : JSON.parse(responseBody);
 
-      return parsedBody;
-    } catch (e: any) {
-      const stack = e.stack;
-      httpErrorHandler(e, stack, constructURL.toString());
+    if (!response.ok) {
+      throw new MeiliSearchApiError(response, parsedResponse);
     }
+
+    return parsedResponse;
   }
 
   async fetchWithTimeout(

diff --git a/src/indexes.ts b/src/indexes.ts
@@ -9,7 +9,7 @@
 
 import {
   MeiliSearchError,
-  MeiliSearchCommunicationError,
+  MeiliSearchRequestError,
   versionErrorHintMessage,
   MeiliSearchApiError,
 } from './errors';
@@ -380,7 +380,7 @@ class Index<T extends Record<string, any> = Record<string, any>> {
           Promise<ResourceResults<D[]>>
         >(url, parameters);
       } catch (e) {
-        if (e instanceof MeiliSearchCommunicationError) {
+        if (e instanceof MeiliSearchRequestError) {
           e.message = versionErrorHintMessage(e.message, 'getDocuments');
         } else if (e instanceof MeiliSearchApiError) {
           e.message = versionErrorHintMessage(e.message, 'getDocuments');
@@ -605,10 +605,7 @@ class Index<T extends Record<string, any> = Record<string, any>> {
 
       return new EnqueuedTask(task);
     } catch (e) {
-      if (
-        e instanceof MeiliSearchCommunicationError &&
-        isDocumentsDeletionQuery
-      ) {
+      if (e instanceof MeiliSearchRequestError && isDocumentsDeletionQuery) {
         e.message = versionErrorHintMessage(e.message, 'deleteDocuments');
       } else if (e instanceof MeiliSearchApiError) {
         e.message = versionErrorHintMessage(e.message, 'deleteDocuments');

diff --git a/src/types/types.ts b/src/types/types.ts
@@ -566,7 +566,7 @@ export type TaskObject = Omit<EnqueuedTaskObject, 'taskUid'> & {
     // Query parameters used to filter the tasks
     originalFilter?: string;
   };
-  error: MeiliSearchErrorInfo | null;
+  error: MeiliSearchErrorResponse | null;
   duration: string;
   startedAt: string;
   finishedAt: string;
@@ -675,13 +675,16 @@ export interface FetchError extends Error {
   code: string;
 }
 
-export type MeiliSearchErrorInfo = {
-  code: string;
-  link: string;
+export type MeiliSearchErrorResponse = {
   message: string;
+  // @TODO: Could be typed, but will it be kept updated? https://www.meilisearch.com/docs/reference/errors/error_codes
+  code: string;
+  // @TODO: Could be typed https://www.meilisearch.com/docs/reference/errors/overview#errors
   type: string;
+  link: string;
 };
 
+// @TODO: This doesn't seem to be up to date, and its usefullness comes into question.
 export const ErrorStatusCode = {
   /** @see https://www.meilisearch.com/docs/reference/errors/error_codes#index_creation_failed */
   INDEX_CREATION_FAILED: 'index_creation_failed',