From 017abd1577d289f65d1d5916c1ef2ee5a735faae Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Mon, 22 Aug 2016 17:19:55 -0400 Subject: [PATCH 1/3] Add failing tests that demonstrate the problem --- test/unit/media-error.test.js | 58 +++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 test/unit/media-error.test.js diff --git a/test/unit/media-error.test.js b/test/unit/media-error.test.js new file mode 100644 index 0000000000..b1c1796500 --- /dev/null +++ b/test/unit/media-error.test.js @@ -0,0 +1,58 @@ +/* eslint-env qunit */ +import window from 'global/window'; +import MediaError from '../../src/js/media-error'; + +/** + * Creates a real native MediaError object. + * + * @param {Number} code + * @param {String} [message] + * @return {MediaError} + */ +const createNativeMediaError = (code, message) => { + const err = Object.create(window.MediaError); + + Object.defineProperty(err, 'code', {value: code}); + + if (message) { + err.message = message; + } + + return err; +}; + +QUnit.module('MediaError'); + +QUnit.test('can be constructed from a number', function(assert) { + const mediaError = new MediaError(1); + + assert.strictEqual(mediaError.code, 1); + assert.strictEqual(mediaError.message, MediaError.defaultMessages['1']); +}); + +QUnit.test('can be constructed from a string', function(assert) { + const mediaError = new MediaError('hello, world'); + + assert.strictEqual(mediaError.code, 0); + assert.strictEqual(mediaError.message, 'hello, world'); +}); + +QUnit.test('can be constructed from an object', function(assert) { + const mediaError = new MediaError({code: 2}); + const mediaErrorMsg = new MediaError({code: 2, message: 'hello, world'}); + + assert.strictEqual(mediaError.code, 2); + assert.strictEqual(mediaError.message, MediaError.defaultMessages['2']); + assert.strictEqual(mediaErrorMsg.code, 2); + assert.strictEqual(mediaErrorMsg.message, 'hello, world'); +}); + +QUnit.test('can be constructed from a native MediaError object', function(assert) { + const mediaError = new MediaError(createNativeMediaError(3)); + const mediaErrorMsg = new MediaError(createNativeMediaError(4, 'hello, world')); + + assert.strictEqual(mediaError.code, 3); + assert.strictEqual(mediaError.message, MediaError.defaultMessages['3']); + assert.strictEqual(mediaErrorMsg.code, 4); + assert.strictEqual(mediaErrorMsg.message, 'hello, world'); +}); From 9d6ddca00b477ddc1b17afa50df893a1754bb297 Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Mon, 22 Aug 2016 17:24:27 -0400 Subject: [PATCH 2/3] Fix broken behavior --- src/js/media-error.js | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/js/media-error.js b/src/js/media-error.js index 90c684991d..dfed1c74ea 100644 --- a/src/js/media-error.js +++ b/src/js/media-error.js @@ -6,16 +6,23 @@ import assign from 'object.assign'; /* * Custom MediaError to mimic the HTML5 MediaError * - * @param {Number} code The media error code + * @param {Number|String|Object|MediaError} value The media error code */ -let MediaError = function(code){ - if (typeof code === 'number') { - this.code = code; - } else if (typeof code === 'string') { +const MediaError = function(value) { + if (typeof value === 'number') { + this.code = value; + } else if (typeof value === 'string') { // default code is zero, so this is a custom error - this.message = code; - } else if (typeof code === 'object') { // object - assign(this, code); + this.message = value; + } else if (typeof value === 'object') { + + // We assign the `code` property manually because native MediaError objects + // do not expose it as an own/enumerable property of the object. + if (typeof value.code === 'number') { + this.code = value.code; + } + + assign(this, value); } if (!this.message) { From 0253742dcb9793208789c7331910ff04078b6749 Mon Sep 17 00:00:00 2001 From: Pat O'Neill Date: Mon, 22 Aug 2016 17:30:05 -0400 Subject: [PATCH 3/3] Allow redundant MediaError construction --- src/js/media-error.js | 11 +++++++++-- src/js/player.js | 7 +------ src/js/tech/tech.js | 6 +----- test/unit/media-error.test.js | 7 +++++++ 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/js/media-error.js b/src/js/media-error.js index dfed1c74ea..6450cb139e 100644 --- a/src/js/media-error.js +++ b/src/js/media-error.js @@ -8,7 +8,14 @@ import assign from 'object.assign'; * * @param {Number|String|Object|MediaError} value The media error code */ -const MediaError = function(value) { +function MediaError(value) { + + // Allow redundant calls to this constructor to avoid having `instanceof` + // checks peppered around the code. + if (value instanceof MediaError) { + return value; + } + if (typeof value === 'number') { this.code = value; } else if (typeof value === 'string') { @@ -28,7 +35,7 @@ const MediaError = function(value) { if (!this.message) { this.message = MediaError.defaultMessages[this.code] || ''; } -}; +} /* * The error code that refers two one of the defined diff --git a/src/js/player.js b/src/js/player.js index 293c8407bb..4a054928c6 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -2277,12 +2277,7 @@ class Player extends Component { return this; } - // error instance - if (err instanceof MediaError) { - this.error_ = err; - } else { - this.error_ = new MediaError(err); - } + this.error_ = new MediaError(err); // add the vjs-error classname to the player this.addClass('vjs-error'); diff --git a/src/js/tech/tech.js b/src/js/tech/tech.js index 30c1da692b..002c3c6591 100644 --- a/src/js/tech/tech.js +++ b/src/js/tech/tech.js @@ -282,11 +282,7 @@ class Tech extends Component { */ error(err) { if (err !== undefined) { - if (err instanceof MediaError) { - this.error_ = err; - } else { - this.error_ = new MediaError(err); - } + this.error_ = new MediaError(err); this.trigger('error'); } return this.error_; diff --git a/test/unit/media-error.test.js b/test/unit/media-error.test.js index b1c1796500..49a04be8a6 100644 --- a/test/unit/media-error.test.js +++ b/test/unit/media-error.test.js @@ -56,3 +56,10 @@ QUnit.test('can be constructed from a native MediaError object', function(assert assert.strictEqual(mediaErrorMsg.code, 4); assert.strictEqual(mediaErrorMsg.message, 'hello, world'); }); + +QUnit.test('can be constructed redundantly', function(assert) { + const mediaError = new MediaError(2); + const redundantMediaError = new MediaError(mediaError); + + assert.strictEqual(redundantMediaError, mediaError); +});