Skip to content

Commit

Permalink
Implement better error message for the v4
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
peaBerberian committed Sep 26, 2023
1 parent 395f6a4 commit cfaf0fd
Show file tree
Hide file tree
Showing 17 changed files with 34 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.";

/**
Expand Down
4 changes: 2 additions & 2 deletions src/core/decrypt/__tests__/__global__/media_keys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down
4 changes: 2 additions & 2 deletions src/errors/__tests__/encrypted_media_error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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");
});
});
2 changes: 1 addition & 1 deletion src/errors/__tests__/error_message.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
4 changes: 2 additions & 2 deletions src/errors/__tests__/format_error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});

Expand All @@ -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");
});
});
6 changes: 3 additions & 3 deletions src/errors/__tests__/media_error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -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");
});
});
4 changes: 2 additions & 2 deletions src/errors/__tests__/network_error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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");
});

Expand Down
6 changes: 3 additions & 3 deletions src/errors/__tests__/other_error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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", () => {
Expand All @@ -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");
});
});
1 change: 0 additions & 1 deletion src/errors/custom_loader_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,3 @@ export default class CustomLoaderError extends Error {
this.xhr = xhr;
}
}

6 changes: 4 additions & 2 deletions src/errors/encrypted_media_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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") {
Expand Down
9 changes: 2 additions & 7 deletions src/errors/error_message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
}
2 changes: 1 addition & 1 deletion src/errors/media_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/errors/network_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/errors/other_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
8 changes: 4 additions & 4 deletions src/manifest/__tests__/update_periods.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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 */
Expand All @@ -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 */
Expand All @@ -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");
});
});

0 comments on commit cfaf0fd

Please sign in to comment.