From cfaf0fdb406cd18fce4ff8435b6ff6797a3ea513 Mon Sep 17 00:00:00 2001 From: Paul Berberian Date: Tue, 26 Sep 2023 15:09:23 +0200 Subject: [PATCH] Implement better error message for the v4 Until now, the RxPlayer's errors set the Error's `name` property inside its `message` property. When logged in the console, such errors are automatically formatted by the browser which generally choose to re-use the `name` to produce a meaningful error string. This lead to a repeat like: `EncryptedMediaError: EncryptedMediaError (INCOMPATIBLE_KEYSYSTEMS) Some description message` This was unnecessary and kind of ugly but we were afraid some apps were for whatever reasons exploiting the `message` string and thus did not change it in the v3. For the `v4` we choose a more consize message: The code, followed by a colon, followed by a description of what happened. As such, the previous example would look like in a JS console when logged: `EncryptedMediaError: INCOMPATIBLE_KEYSYSTEMS: Some description message` If applications want to detect the error's name/type they can respectively rely on the more appropriate `name` or `type` property (basically the `name` is the PascalCased name here to have an idiomatic JS error, `type` is the RxPlayer's documented UPPER_SNAKE_CASE version of the name). Note that the error's message is still not part of the API and applications should still expect that it could theoretically change its format at any time, even if we're generally (too?) careful with that sort of things. --- .../__tests__/__global__/media_key_system_access.test.ts | 2 +- src/core/decrypt/__tests__/__global__/media_keys.test.ts | 4 ++-- src/errors/__tests__/encrypted_media_error.test.ts | 4 ++-- src/errors/__tests__/error_message.test.ts | 2 +- src/errors/__tests__/format_error.test.ts | 4 ++-- src/errors/__tests__/media_error.test.ts | 6 +++--- src/errors/__tests__/network_error.test.ts | 4 ++-- src/errors/__tests__/other_error.test.ts | 6 +++--- src/errors/custom_loader_error.ts | 1 - src/errors/encrypted_media_error.ts | 6 ++++-- src/errors/error_message.ts | 9 ++------- src/errors/media_error.ts | 2 +- src/errors/network_error.ts | 2 +- src/errors/other_error.ts | 2 +- src/manifest/__tests__/update_periods.test.ts | 8 ++++---- .../utils/__tests__/update_segment_timeline.test.ts | 3 +-- .../utils/__tests__/check_isobmff_integrity.test.ts | 8 ++++---- 17 files changed, 34 insertions(+), 39 deletions(-) diff --git a/src/core/decrypt/__tests__/__global__/media_key_system_access.test.ts b/src/core/decrypt/__tests__/__global__/media_key_system_access.test.ts index 94e823dd236..98368773d31 100644 --- a/src/core/decrypt/__tests__/__global__/media_key_system_access.test.ts +++ b/src/core/decrypt/__tests__/__global__/media_key_system_access.test.ts @@ -48,7 +48,7 @@ export function requestMediaKeySystemAccessNoMediaKeys( } const incompatibleMKSAErrorMessage = - "EncryptedMediaError (INCOMPATIBLE_KEYSYSTEMS) No key system compatible " + + "INCOMPATIBLE_KEYSYSTEMS: No key system compatible " + "with your wanted configuration has been found in the current browser."; /** diff --git a/src/core/decrypt/__tests__/__global__/media_keys.test.ts b/src/core/decrypt/__tests__/__global__/media_keys.test.ts index 101659726d6..1bcbc5b19c9 100644 --- a/src/core/decrypt/__tests__/__global__/media_keys.test.ts +++ b/src/core/decrypt/__tests__/__global__/media_keys.test.ts @@ -71,7 +71,7 @@ describe("core - decrypt - global tests - media key system access", () => { ksConfig); expect(error).toBeInstanceOf(Error); expect(error.message).toEqual( - "EncryptedMediaError (CREATE_MEDIA_KEYS_ERROR) No non no" + "CREATE_MEDIA_KEYS_ERROR: No non no" ); expect(error.name).toEqual("EncryptedMediaError"); expect(error.code).toEqual("CREATE_MEDIA_KEYS_ERROR"); @@ -100,7 +100,7 @@ describe("core - decrypt - global tests - media key system access", () => { ksConfig); expect(error).toBeInstanceOf(Error); expect(error.message).toEqual( - "EncryptedMediaError (CREATE_MEDIA_KEYS_ERROR) No non no" + "CREATE_MEDIA_KEYS_ERROR: No non no" ); expect(error.name).toEqual("EncryptedMediaError"); expect(error.code).toEqual("CREATE_MEDIA_KEYS_ERROR"); diff --git a/src/errors/__tests__/encrypted_media_error.test.ts b/src/errors/__tests__/encrypted_media_error.test.ts index 670ff92093d..6411dac2285 100644 --- a/src/errors/__tests__/encrypted_media_error.test.ts +++ b/src/errors/__tests__/encrypted_media_error.test.ts @@ -26,7 +26,7 @@ describe("errors - EncryptedMediaError", () => { expect(encryptedMediaError.code).toBe("KEY_LOAD_TIMEOUT"); expect(encryptedMediaError.fatal).toBe(false); expect(encryptedMediaError.message) - .toBe("EncryptedMediaError (KEY_LOAD_TIMEOUT) test"); + .toBe("KEY_LOAD_TIMEOUT: test"); }); it("should be able to set it as fatal", () => { @@ -40,6 +40,6 @@ describe("errors - EncryptedMediaError", () => { expect(encryptedMediaError.code).toBe("INCOMPATIBLE_KEYSYSTEMS"); expect(encryptedMediaError.fatal).toBe(true); expect(encryptedMediaError.message) - .toBe("EncryptedMediaError (INCOMPATIBLE_KEYSYSTEMS) test"); + .toBe("INCOMPATIBLE_KEYSYSTEMS: test"); }); }); diff --git a/src/errors/__tests__/error_message.test.ts b/src/errors/__tests__/error_message.test.ts index 4cc1398a691..7c4d053722b 100644 --- a/src/errors/__tests__/error_message.test.ts +++ b/src/errors/__tests__/error_message.test.ts @@ -18,6 +18,6 @@ import errorMessage from "../error_message"; describe("Errors - generateErrorMessage", () => { it("should format a readable error message", () => { - expect(errorMessage("foo", "bar", "baz")).toBe("foo (bar) baz"); + expect(errorMessage("bar", "baz")).toBe("bar: baz"); }); }); diff --git a/src/errors/__tests__/format_error.test.ts b/src/errors/__tests__/format_error.test.ts index a7fb2d037e3..a313a6e62c3 100644 --- a/src/errors/__tests__/format_error.test.ts +++ b/src/errors/__tests__/format_error.test.ts @@ -43,7 +43,7 @@ describe("errors - formatError", () => { const formattedError = formatError(error1, { defaultCode: "toto", defaultReason: "a" }); expect(formattedError).toBeInstanceOf(OtherError); - expect(formattedError.message).toBe("OtherError (toto) Error: Abcdef"); + expect(formattedError.message).toBe("toto: Error: Abcdef"); expect(formattedError.code).toBe("toto"); }); @@ -56,7 +56,7 @@ describe("errors - formatError", () => { const formattedError = formatError(error1, { defaultCode: "toto", defaultReason: "a" }); expect(formattedError).toBeInstanceOf(OtherError); - expect(formattedError.message).toBe("OtherError (toto) a"); + expect(formattedError.message).toBe("toto: a"); expect(formattedError.code).toBe("toto"); }); }); diff --git a/src/errors/__tests__/media_error.test.ts b/src/errors/__tests__/media_error.test.ts index 782601397ab..0b6f1de1142 100644 --- a/src/errors/__tests__/media_error.test.ts +++ b/src/errors/__tests__/media_error.test.ts @@ -25,7 +25,7 @@ describe("errors - MediaError", () => { expect(mediaError.type).toBe("MEDIA_ERROR"); expect(mediaError.code).toBe("MEDIA_TIME_BEFORE_MANIFEST"); expect(mediaError.fatal).toBe(false); - expect(mediaError.message).toBe("MediaError (MEDIA_TIME_BEFORE_MANIFEST) test"); + expect(mediaError.message).toBe("MEDIA_TIME_BEFORE_MANIFEST: test"); }); it("should be able to set it as fatal", () => { @@ -37,7 +37,7 @@ describe("errors - MediaError", () => { expect(mediaError.type).toBe("MEDIA_ERROR"); expect(mediaError.code).toBe("MEDIA_TIME_AFTER_MANIFEST"); expect(mediaError.fatal).toBe(true); - expect(mediaError.message).toBe("MediaError (MEDIA_TIME_AFTER_MANIFEST) test"); + expect(mediaError.message).toBe("MEDIA_TIME_AFTER_MANIFEST: test"); }); it("should filter in a valid error code", () => { @@ -50,6 +50,6 @@ describe("errors - MediaError", () => { expect(mediaError.type).toBe("MEDIA_ERROR"); expect(mediaError.code).toBe("MEDIA_ERR_NETWORK"); expect(mediaError.fatal).toBe(true); - expect(mediaError.message).toBe("MediaError (MEDIA_ERR_NETWORK) test"); + expect(mediaError.message).toBe("MEDIA_ERR_NETWORK: test"); }); }); diff --git a/src/errors/__tests__/network_error.test.ts b/src/errors/__tests__/network_error.test.ts index 529b380eec6..3dde53b64e8 100644 --- a/src/errors/__tests__/network_error.test.ts +++ b/src/errors/__tests__/network_error.test.ts @@ -29,7 +29,7 @@ describe("errors - NetworkError", () => { expect(networkError.code).toBe("PIPELINE_LOAD_ERROR"); expect(networkError.fatal).toBe(false); expect(networkError.message) - .toBe("NetworkError (PIPELINE_LOAD_ERROR) The request timed out"); + .toBe("PIPELINE_LOAD_ERROR: The request timed out"); }); it("should filter in a valid error code", () => { @@ -44,7 +44,7 @@ describe("errors - NetworkError", () => { expect(networkError.code).toBe("PIPELINE_LOAD_ERROR"); expect(networkError.fatal).toBe(true); expect(networkError.message) - .toBe("NetworkError (PIPELINE_LOAD_ERROR) An HTTP status code " + + .toBe("PIPELINE_LOAD_ERROR: An HTTP status code " + "indicating failure was received: 403"); }); diff --git a/src/errors/__tests__/other_error.test.ts b/src/errors/__tests__/other_error.test.ts index 9512d2d5e73..fb33a0fc7ca 100644 --- a/src/errors/__tests__/other_error.test.ts +++ b/src/errors/__tests__/other_error.test.ts @@ -24,7 +24,7 @@ describe("errors - OtherError", () => { expect(otherError.type).toBe("OTHER_ERROR"); expect(otherError.code).toBe("NONE"); expect(otherError.fatal).toBe(false); - expect(otherError.message).toBe("OtherError (NONE) tata"); + expect(otherError.message).toBe("NONE: tata"); }); it("should be able to set it as fatal", () => { @@ -36,7 +36,7 @@ describe("errors - OtherError", () => { expect(otherError.type).toBe("OTHER_ERROR"); expect(otherError.code).toBe("NONE"); expect(otherError.fatal).toBe(true); - expect(otherError.message).toBe("OtherError (NONE) test"); + expect(otherError.message).toBe("NONE: test"); }); it("should filter in a valid error code", () => { @@ -48,6 +48,6 @@ describe("errors - OtherError", () => { expect(otherError.type).toBe("OTHER_ERROR"); expect(otherError.code).toBe("PIPELINE_LOAD_ERROR"); expect(otherError.fatal).toBe(true); - expect(otherError.message).toBe("OtherError (PIPELINE_LOAD_ERROR) test"); + expect(otherError.message).toBe("PIPELINE_LOAD_ERROR: test"); }); }); diff --git a/src/errors/custom_loader_error.ts b/src/errors/custom_loader_error.ts index dfa3f33877e..99c41641c9f 100644 --- a/src/errors/custom_loader_error.ts +++ b/src/errors/custom_loader_error.ts @@ -50,4 +50,3 @@ export default class CustomLoaderError extends Error { this.xhr = xhr; } } - diff --git a/src/errors/encrypted_media_error.ts b/src/errors/encrypted_media_error.ts index 92bd31c6a82..0d1e7b090e5 100644 --- a/src/errors/encrypted_media_error.ts +++ b/src/errors/encrypted_media_error.ts @@ -51,7 +51,9 @@ export default class EncryptedMediaError extends Error { constructor( code : IEncryptedMediaErrorCode, reason : string, - supplementaryInfos? : { keyStatuses? : IEncryptedMediaErrorKeyStatusObject[] } + supplementaryInfos? : { keyStatuses? : IEncryptedMediaErrorKeyStatusObject[] | + undefined; } | + undefined ) { super(); // @see https://stackoverflow.com/questions/41102060/typescript-extending-error-class @@ -61,7 +63,7 @@ export default class EncryptedMediaError extends Error { this.type = ErrorTypes.ENCRYPTED_MEDIA_ERROR; this.code = code; - this.message = errorMessage(this.name, this.code, reason); + this.message = errorMessage(this.code, reason); this.fatal = false; if (typeof supplementaryInfos?.keyStatuses === "string") { diff --git a/src/errors/error_message.ts b/src/errors/error_message.ts index b807109d196..90712060822 100644 --- a/src/errors/error_message.ts +++ b/src/errors/error_message.ts @@ -16,15 +16,10 @@ /** * Generate a normalized error message. - * @param {string} name * @param {string} code * @param {Error|string|Event|null} [reason] * @returns {string} */ -export default function errorMessage( - name : string, - code : string, - reason : string -) : string { - return `${name} (${code}) ${reason}`; +export default function errorMessage(code : string, reason : string) : string { + return `${code}: ${reason}`; } diff --git a/src/errors/media_error.ts b/src/errors/media_error.ts index 7d1815b6f26..78db32fc965 100644 --- a/src/errors/media_error.ts +++ b/src/errors/media_error.ts @@ -95,7 +95,7 @@ export default class MediaError extends Error { this.type = ErrorTypes.MEDIA_ERROR; this.code = code; - this.message = errorMessage(this.name, this.code, reason); + this.message = errorMessage(this.code, reason); this.fatal = false; const adaptations = context?.adaptations; if (adaptations !== undefined && adaptations.length > 0) { diff --git a/src/errors/network_error.ts b/src/errors/network_error.ts index a3ca3ff323e..7f6af5391f4 100644 --- a/src/errors/network_error.ts +++ b/src/errors/network_error.ts @@ -56,7 +56,7 @@ export default class NetworkError extends Error { this.errorType = baseError.type; this.code = code; - this.message = errorMessage(this.name, this.code, baseError.message); + this.message = errorMessage(this.code, baseError.message); this.fatal = false; } diff --git a/src/errors/other_error.ts b/src/errors/other_error.ts index 5bbcf490b81..067fa6a59f6 100644 --- a/src/errors/other_error.ts +++ b/src/errors/other_error.ts @@ -44,7 +44,7 @@ export default class OtherError extends Error { this.type = ErrorTypes.OTHER_ERROR; this.code = code; - this.message = errorMessage(this.name, this.code, reason); + this.message = errorMessage(this.code, reason); this.fatal = false; } } diff --git a/src/manifest/__tests__/update_periods.test.ts b/src/manifest/__tests__/update_periods.test.ts index c4f0b018c32..4ca01aa7961 100644 --- a/src/manifest/__tests__/update_periods.test.ts +++ b/src/manifest/__tests__/update_periods.test.ts @@ -410,7 +410,7 @@ describe("Manifest - updatePeriods", () => { expect((error as { type? : string }).type).toEqual("MEDIA_ERROR"); expect((error as { code? : string }).code).toEqual("MANIFEST_UPDATE_ERROR"); expect(error.message).toEqual( - "MediaError (MANIFEST_UPDATE_ERROR) Cannot perform partial update: not enough data" + "MANIFEST_UPDATE_ERROR: Cannot perform partial update: not enough data" ); expect(oldPeriods.length).toBe(1); expect(oldPeriods[0].id).toBe("p1"); @@ -511,7 +511,7 @@ describe("Manifest - updatePeriods", () => { expect((error as { type? : string }).type).toEqual("MEDIA_ERROR"); expect((error as { code? : string }).code).toEqual("MANIFEST_UPDATE_ERROR"); expect(error.message).toEqual( - "MediaError (MANIFEST_UPDATE_ERROR) Cannot perform partial update: incoherent data" + "MANIFEST_UPDATE_ERROR: Cannot perform partial update: incoherent data" ); expect(oldPeriods.length).toBe(1); expect(oldPeriods[0].id).toBe("p2"); @@ -602,7 +602,7 @@ describe("Manifest - updatePeriods", () => { expect((error as { type? : string }).type).toEqual("MEDIA_ERROR"); expect((error as { code? : string }).code).toEqual("MANIFEST_UPDATE_ERROR"); expect(error.message).toEqual( - "MediaError (MANIFEST_UPDATE_ERROR) Cannot perform partial update: not enough data" + "MANIFEST_UPDATE_ERROR: Cannot perform partial update: not enough data" ); expect(oldPeriods.length).toBe(2); expect(oldPeriods[0].id).toBe("p0"); @@ -674,7 +674,7 @@ describe("Manifest - updatePeriods", () => { expect((error as { type? : string }).type).toEqual("MEDIA_ERROR"); expect((error as { code? : string }).code).toEqual("MANIFEST_UPDATE_ERROR"); expect(error.message).toEqual( - "MediaError (MANIFEST_UPDATE_ERROR) Cannot perform partial update: incoherent data" + "MANIFEST_UPDATE_ERROR: Cannot perform partial update: incoherent data" ); expect(oldPeriods.length).toBe(1); expect(oldPeriods[0].id).toBe("p1"); diff --git a/src/parsers/manifest/utils/__tests__/update_segment_timeline.test.ts b/src/parsers/manifest/utils/__tests__/update_segment_timeline.test.ts index 763ff43b239..7aaa10985d4 100644 --- a/src/parsers/manifest/utils/__tests__/update_segment_timeline.test.ts +++ b/src/parsers/manifest/utils/__tests__/update_segment_timeline.test.ts @@ -107,8 +107,7 @@ describe("Manifest Parsers utils - updateSegmentTimeline", () => { expect((err as { type? : string }).type).toEqual("MEDIA_ERROR"); expect((err as { code? : string }).code).toEqual("MANIFEST_UPDATE_ERROR"); expect(err.message) - .toEqual("MediaError (MANIFEST_UPDATE_ERROR) Cannot perform " + - "partial update: not enough data"); + .toEqual("MANIFEST_UPDATE_ERROR: Cannot perform partial update: not enough data"); expect(oldTimeline1).toEqual(oldTimeline1Cloned); expect(mockLogWarn).not.toHaveBeenCalled(); }); diff --git a/src/transports/utils/__tests__/check_isobmff_integrity.test.ts b/src/transports/utils/__tests__/check_isobmff_integrity.test.ts index 486fd159b4d..c6f2b5eaa0d 100644 --- a/src/transports/utils/__tests__/check_isobmff_integrity.test.ts +++ b/src/transports/utils/__tests__/check_isobmff_integrity.test.ts @@ -72,7 +72,7 @@ describe("transports utils - checkISOBMFFIntegrity", () => { expect((error as typeof OtherError).type).toEqual("OTHER_ERROR"); expect((error as typeof OtherError).code).toEqual("INTEGRITY_ERROR"); expect((error as typeof OtherError).message) - .toEqual("OtherError (INTEGRITY_ERROR) Incomplete `ftyp` box"); + .toEqual("INTEGRITY_ERROR: Incomplete `ftyp` box"); }); it("should throw an other error if an init segment is missing a complete moov", () => { @@ -95,7 +95,7 @@ describe("transports utils - checkISOBMFFIntegrity", () => { expect((error as typeof OtherError).type).toEqual("OTHER_ERROR"); expect((error as typeof OtherError).code).toEqual("INTEGRITY_ERROR"); expect((error as typeof OtherError).message) - .toEqual("OtherError (INTEGRITY_ERROR) Incomplete `moov` box"); + .toEqual("INTEGRITY_ERROR: Incomplete `moov` box"); }); /* eslint-disable max-len */ @@ -119,7 +119,7 @@ describe("transports utils - checkISOBMFFIntegrity", () => { expect((error as typeof OtherError).type).toEqual("OTHER_ERROR"); expect((error as typeof OtherError).code).toEqual("INTEGRITY_ERROR"); expect((error as typeof OtherError).message) - .toEqual("OtherError (INTEGRITY_ERROR) Incomplete `moof` box"); + .toEqual("INTEGRITY_ERROR: Incomplete `moof` box"); }); /* eslint-disable max-len */ @@ -143,6 +143,6 @@ describe("transports utils - checkISOBMFFIntegrity", () => { expect((error as typeof OtherError).type).toEqual("OTHER_ERROR"); expect((error as typeof OtherError).code).toEqual("INTEGRITY_ERROR"); expect((error as typeof OtherError).message) - .toEqual("OtherError (INTEGRITY_ERROR) Incomplete `mdat` box"); + .toEqual("INTEGRITY_ERROR: Incomplete `mdat` box"); }); });