From d46faeb92641f5189abee9b707851546ed98d5df Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Thu, 22 Dec 2016 19:46:35 -0500 Subject: [PATCH 1/3] Fix extra warn logs on already initialized player references --- src/js/video.js | 2 +- test/unit/video.test.js | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/js/video.js b/src/js/video.js index 96e79d7cc7..2aaa99b067 100644 --- a/src/js/video.js +++ b/src/js/video.js @@ -80,7 +80,7 @@ function videojs(id, options, ready) { if (videojs.getPlayers()[id]) { // If options or ready funtion are passed, warn - if (options) { + if (Object.keys(options).length) { log.warn(`Player "${id}" is already initialised. Options will not be applied.`); } diff --git a/test/unit/video.test.js b/test/unit/video.test.js index c9252613c1..65af975ec0 100644 --- a/test/unit/video.test.js +++ b/test/unit/video.test.js @@ -2,6 +2,7 @@ import videojs from '../../src/js/video.js'; import TestHelpers from './test-helpers.js'; import * as Dom from '../../src/js/utils/dom.js'; +import log from '../../src/js/utils/log.js'; import document from 'global/document'; QUnit.module('video.js'); @@ -45,6 +46,42 @@ QUnit.test('should return a video player instance', function(assert) { player2.dispose(); }); +QUnit.test('should log about already initalized players if options already passed', +function(assert) { + const origWarnLog = log.warn; + const fixture = document.getElementById('qunit-fixture'); + const warnLogs = []; + + log.warn = (args) => { + warnLogs.push(args); + }; + + fixture.innerHTML += ''; + + const player = videojs('test_vid_id', { techOrder: ['techFaker'] }); + + assert.ok(player, 'created player from tag'); + assert.ok(player.id() === 'test_vid_id'); + assert.ok(!warnLogs.length, 'no warn logs'); + + const playerAgain = videojs('test_vid_id'); + + assert.ok(player === playerAgain, 'did not create a second player from same tag'); + assert.ok(!warnLogs.length, 'no warn logs'); + + const playerAgainWithOptions = videojs('test_vid_id', { techOrder: ['techFaker'] }); + + assert.ok(player === playerAgainWithOptions, + 'did not create a second player from same tag'); + assert.equal(warnLogs.length, 1, 'logged a warning'); + assert.equal(warnLogs[0], + 'Player "test_vid_id" is already initialised. ' + + 'Options will not be applied.', + 'logged the right message'); + + log.warn = origWarnLog; +}); + QUnit.test('should return a video player instance from el html5 tech', function(assert) { const fixture = document.getElementById('qunit-fixture'); From 7642488a254a75d01f24b3926caf241df1397d5a Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Thu, 22 Dec 2016 22:42:26 -0500 Subject: [PATCH 2/3] Move options check for player initialization references --- src/js/video.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/video.js b/src/js/video.js index 2aaa99b067..9b3e19e91c 100644 --- a/src/js/video.js +++ b/src/js/video.js @@ -65,8 +65,6 @@ if (typeof HTMLVideoElement === 'undefined' && function videojs(id, options, ready) { let tag; - options = options || {}; - // Allow for element or ID to be passed in // String ID if (typeof id === 'string') { @@ -80,7 +78,7 @@ function videojs(id, options, ready) { if (videojs.getPlayers()[id]) { // If options or ready funtion are passed, warn - if (Object.keys(options).length) { + if (options) { log.warn(`Player "${id}" is already initialised. Options will not be applied.`); } @@ -111,6 +109,8 @@ function videojs(id, options, ready) { return tag.player || Player.players[tag.playerId]; } + options = options || {}; + videojs.hooks('beforesetup').forEach(function(hookFunction) { const opts = hookFunction(tag, mergeOptions(options)); From 70f5ff258d11614e7ee78ec11d11c7df802f1463 Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Thu, 22 Dec 2016 23:00:14 -0500 Subject: [PATCH 3/3] Clean up unit test --- test/unit/video.test.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/unit/video.test.js b/test/unit/video.test.js index 65af975ec0..2472d06600 100644 --- a/test/unit/video.test.js +++ b/test/unit/video.test.js @@ -61,25 +61,27 @@ function(assert) { const player = videojs('test_vid_id', { techOrder: ['techFaker'] }); assert.ok(player, 'created player from tag'); - assert.ok(player.id() === 'test_vid_id'); - assert.ok(!warnLogs.length, 'no warn logs'); + assert.equal(player.id(), 'test_vid_id', 'player has the right ID'); + assert.equal(warnLogs.length, 0, 'no warn logs'); const playerAgain = videojs('test_vid_id'); - assert.ok(player === playerAgain, 'did not create a second player from same tag'); - assert.ok(!warnLogs.length, 'no warn logs'); + assert.equal(player, playerAgain, 'did not create a second player from same tag'); + assert.equal(warnLogs.length, 0, 'no warn logs'); const playerAgainWithOptions = videojs('test_vid_id', { techOrder: ['techFaker'] }); - assert.ok(player === playerAgainWithOptions, - 'did not create a second player from same tag'); + assert.equal(player, + playerAgainWithOptions, + 'did not create a second player from same tag'); assert.equal(warnLogs.length, 1, 'logged a warning'); assert.equal(warnLogs[0], - 'Player "test_vid_id" is already initialised. ' + - 'Options will not be applied.', + 'Player "test_vid_id" is already initialised. Options will not be applied.', 'logged the right message'); log.warn = origWarnLog; + + player.dispose(); }); QUnit.test('should return a video player instance from el html5 tech', function(assert) {