From 3340014639daef63027a374d31b522e6b5621741 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Thu, 4 May 2023 23:10:23 -0700 Subject: [PATCH 01/14] fix: onRequest hooks called too late --- README.md | 49 +++++++++++++++++++++++++++++++++++++------------ src/xhr.js | 5 ++--- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 3f260a904..5bf0266f5 100644 --- a/README.md +++ b/README.md @@ -644,20 +644,35 @@ callback function as a parameter as well as `offRequest` and `offResponse` functions which will remove a callback function from the `onRequest` or `onResponse` set if it exists. -The `onRequest(callback)` function takes a `callback` function that will pass the xhr `request` -Object to that callback. These callbacks are called synchronously, in the order registered -and act as pre-request hooks for modifying the xhr `request` Object prior to making a request. +The `onRequest(callback)` function takes a `callback` function that will pass the xhr `options` +Object to that callback. These callbacks are called synchronously, in the order registered +and act as pre-request hooks for modifying the xhr `options` Object prior to making a request. +https://github.com/videojs/xhr#options Example: ```javascript -const playerRequestHook = (request) => { - const requestUrl = new URL(request.uri); +const playerRequestHook = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.uri = requestUrl.href; + options.uri = requestUrl.href; }; player.tech().vhs.xhr.onRequest(playerRequestHook); ``` +If access to the `xhr` Object is required prior to the `xhr.send` call, an `options.beforeSend` +callback can be set within an `onRequest` callback function that will pass the `xhr` Object +as a parameter and will be called immediately prior to `xhr.send`. + +Example: +```javascript +const playerXhrRequestHook = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://new.uri'); + }; +}; +player.tech().vhs.xhr.onRequest(playerXhrRequestHook); +``` + The `onResponse(callback)` function takes a `callback` function that will pass the xhr `request`, `error`, and `response` Objects to that callback. These callbacks are called in the order registered and act as post-request hooks for gathering data from the @@ -704,22 +719,32 @@ player.tech().vhs.xhr.beforeRequest = function(options) { The global `videojs.Vhs` also exposes an `xhr` property. Adding `onRequest`, `onResponse` hooks and/or specifying a `beforeRequest` -function that will allow you to intercept the request Object, response -data and options for *all* requests in every player on a page. For -consistency across browsers the video source should be set at runtime +function that will allow you to intercept the request options, xhr +Object, error and response data for *all* requests in every player on a page. +For consistency across browsers the video source should be set at runtime once the video player is ready. Example: ```javascript // Global request callback, will affect every player. -const globalRequestHook = (request) => { - const requestUrl = new URL(request.uri); +const globalRequestHook = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.uri = requestUrl.href; + options.uri = requestUrl.href; }; videojs.Vhs.xhr.onRequest(globalRequestHook); ``` +```javascript +// Global request callback defining beforeSend function, will affect every player. +const globalXhrRequestHook = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://new.uri'); + }; +}; +videojs.Vhs.xhr.onRequest(globalXhrRequestHook); +``` + ```javascript // Global response hook callback, will affect every player. const globalResponseHook = (request, error, response) => { diff --git a/src/xhr.js b/src/xhr.js index 4cb45fde6..30b09d741 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -99,6 +99,8 @@ const xhrFactory = function() { // TODO: switch back to videojs.Vhs.xhr.name === 'XhrFunction' when we drop IE11 const xhrMethod = videojs.Vhs.xhr.original === true ? videojsXHR : videojs.Vhs.xhr; + // call all registered onRequest hooks + callAllHooks(_requestCallbackSet, options); const request = xhrMethod(options, function(error, response) { // call all registered onResponse hooks callAllHooks(_responseCallbackSet, request, error, response); @@ -112,9 +114,6 @@ const xhrFactory = function() { }; request.uri = options.uri; request.requestTime = Date.now(); - // call all registered onRequest hooks - callAllHooks(_requestCallbackSet, request); - return request; }; From cdfa622ead643aca2241e45a3bf7262b788470ca Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Fri, 5 May 2023 10:27:53 -0700 Subject: [PATCH 02/14] fix test hooks and docs --- README.md | 3 +- test/videojs-http-streaming.test.js | 116 +++++++++++++++++++++++----- 2 files changed, 97 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 5bf0266f5..409dbc383 100644 --- a/README.md +++ b/README.md @@ -646,8 +646,7 @@ functions which will remove a callback function from the `onRequest` or The `onRequest(callback)` function takes a `callback` function that will pass the xhr `options` Object to that callback. These callbacks are called synchronously, in the order registered -and act as pre-request hooks for modifying the xhr `options` Object prior to making a request. -https://github.com/videojs/xhr#options +and act as pre-request hooks for modifying the xhr `options` Object prior to making a request. Example: ```javascript diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index e943ab2eb..836a035be 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4114,12 +4114,11 @@ QUnit.test('Allows setting onRequest hooks globally', function(assert) { this.clock.tick(1); openMediaSource(this.player, this.clock); - const globalRequestHook1 = (request) => { - const requestUrl = new URL(request.url); + const globalRequestHook1 = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.url = decodeURIComponent(requestUrl.href); - actualRequestUrl = request.url; + actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; }; const globalRequestHook2 = () => { @@ -4151,12 +4150,11 @@ QUnit.test('Allows setting onRequest hooks on the player', function(assert) { this.clock.tick(1); openMediaSource(this.player, this.clock); - const playerRequestHook1 = (request) => { - const requestUrl = new URL(request.url); + const playerRequestHook1 = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.url = decodeURIComponent(requestUrl.href); - actualRequestUrl = request.url; + actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; }; const playerRequestHook2 = () => { @@ -4193,12 +4191,11 @@ QUnit.test('Allows setting onRequest hooks globally and overriding with player h this.clock.tick(1); openMediaSource(this.player, this.clock); - const globalRequestHook1 = (request) => { - const requestUrl = new URL(request.url); + const globalRequestHook1 = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.url = decodeURIComponent(requestUrl.href); - actualRequestUrlGlobal = request.url; + actualRequestUrlGlobal = options.uri = requestUrl.href; onRequestHookCallCountGlobal++; }; const globalRequestHook2 = () => { @@ -4208,12 +4205,11 @@ QUnit.test('Allows setting onRequest hooks globally and overriding with player h videojs.Vhs.xhr.onRequest(globalRequestHook1); videojs.Vhs.xhr.onRequest(globalRequestHook2); - const playerRequestHook1 = (request) => { - const requestUrl = new URL(request.url); + const playerRequestHook1 = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('bar', 'foo'); - request.url = decodeURIComponent(requestUrl.href); - actualRequestUrlPlayer = request.url; + actualRequestUrlPlayer = options.uri = requestUrl.href; onRequestHookCallCountPlayer++; }; const playerRequestHook2 = () => { @@ -4259,12 +4255,11 @@ QUnit.test('Allows removing onRequest hooks globally with offRequest', function( this.clock.tick(1); openMediaSource(this.player, this.clock); - const globalRequestHook1 = (request) => { - const requestUrl = new URL(request.url); + const globalRequestHook1 = (options) => { + const requestUrl = new URL(options.uri); requestUrl.searchParams.set('foo', 'bar'); - request.url = decodeURIComponent(requestUrl.href); - actualRequestUrl = request.url; + actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; }; const globalRequestHook2 = () => { @@ -4447,6 +4442,87 @@ QUnit.test('Allows removing onResponse hooks globally with offResponse', functio videojs.Vhs.xhr.offResponse(globalResponseHook2); }); +QUnit.test('Allows xhr object access in global onRequest hooks', function(assert) { + let onRequestHookCallCount = 0; + let expectedUrl; + + this.player.src({ + src: 'main.m3u8', + type: 'application/vnd.apple.mpegurl' + }); + + this.clock.tick(1); + + openMediaSource(this.player, this.clock); + const globalXhrRequestHook1 = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://foo.bar'); + }; + onRequestHookCallCount++; + }; + const globalXhrRequestHook2 = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://new.url'); + expectedUrl = xhr.url; + }; + onRequestHookCallCount++; + }; + + videojs.Vhs.xhr.onRequest(globalXhrRequestHook1); + videojs.Vhs.xhr.onRequest(globalXhrRequestHook2); + + // main + this.standardXHRResponse(this.requests.shift()); + + assert.equal(onRequestHookCallCount, 2, '2 onRequest hooks called'); + assert.equal(expectedUrl, 'https://new.url', 'request url modified by onRequest hook calling open on xhr'); + // remove global hooks for other tests + videojs.Vhs.xhr.offRequest(globalXhrRequestHook1); + videojs.Vhs.xhr.offRequest(globalXhrRequestHook2); +}); + +QUnit.test('Allows xhr object access in player onRequest hooks', function(assert) { + let onRequestHookCallCount = 0; + let expectedUrl; + + this.player.src({ + src: 'main.m3u8', + type: 'application/vnd.apple.mpegurl' + }); + + this.clock.tick(1); + + openMediaSource(this.player, this.clock); + const playerXhrRequestHook1 = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://new.url'); + }; + onRequestHookCallCount++; + }; + const playerXhrRequestHook2 = (options) => { + options.beforeSend = (xhr) => { + xhr.open('GET', 'https://foo.bar'); + expectedUrl = xhr.url; + }; + onRequestHookCallCount++; + }; + + // Setup player level xhr hooks. + this.player.tech_.vhs.setupXhrHooks_(); + + this.player.tech_.vhs.xhr.onRequest(playerXhrRequestHook1); + this.player.tech_.vhs.xhr.onRequest(playerXhrRequestHook2); + + // main + this.standardXHRResponse(this.requests.shift()); + + assert.equal(onRequestHookCallCount, 2, '2 onRequest hooks called'); + assert.equal(expectedUrl, 'https://foo.bar', 'request url modified by onRequest hook calling open on xhr'); + // remove player hooks for other tests + this.player.tech_.vhs.xhr.offRequest(playerXhrRequestHook1); + this.player.tech_.vhs.xhr.offRequest(playerXhrRequestHook2); +}); + QUnit.test( 'passes useCueTags vhs option to main playlist controller', function(assert) { From 66865b87a28b97b2d9ef44b8abf9a2a7e122acfd Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Sun, 7 May 2023 21:40:14 -0700 Subject: [PATCH 03/14] add xhr-hooks-ready event --- README.md | 6 ++++++ src/videojs-http-streaming.js | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 409dbc383..cad64378d 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,7 @@ Video.js Compatibility: 7.x, 8.x - [vhs.stats](#vhsstats) - [Events](#events) - [loadedmetadata](#loadedmetadata) + - [xhr-hooks-ready](#xhr-hooks-ready) - [VHS Usage Events](#vhs-usage-events) - [Presence Stats](#presence-stats) - [Use Stats](#use-stats) @@ -820,6 +821,11 @@ are triggered on the player object. Fired after the first segment is downloaded for a playlist. This will not happen until playback if video.js's `metadata` setting is `none` +#### xhr-hooks-ready + +Fired when the player `xhr` object is ready to set `onRequest` and `offRequest` hooks, as well +as remove hooks with `offRequest` and `offResponse`. + ### VHS Usage Events Usage tracking events are fired when we detect a certain HLS feature, encoding setting, diff --git a/src/videojs-http-streaming.js b/src/videojs-http-streaming.js index fc0e60993..add7b6532 100644 --- a/src/videojs-http-streaming.js +++ b/src/videojs-http-streaming.js @@ -1332,6 +1332,12 @@ class VhsHandler extends Component { this.xhr.offResponse = (callback) => { removeOnResponseHook(this.xhr, callback); }; + + /** + * Trigger an event on the player to notify the user that vhs is ready to set xhr hooks. + * This allows hooks to be set before the source is set to vhs when handleSource is called. + */ + this.player_.trigger('xhr-hooks-ready'); } } @@ -1356,7 +1362,6 @@ const VhsSourceHandler = { tech.vhs = new VhsHandler(source, tech, localOptions); tech.vhs.xhr = xhrFactory(); tech.vhs.setupXhrHooks_(); - tech.vhs.src(source.src, source.type); return tech.vhs; }, From 1646e2d8247f85141d64c9a027dd636421765fb4 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Sun, 7 May 2023 22:27:56 -0700 Subject: [PATCH 04/14] add event test --- test/videojs-http-streaming.test.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 836a035be..9570188e1 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4523,6 +4523,26 @@ QUnit.test('Allows xhr object access in player onRequest hooks', function(assert this.player.tech_.vhs.xhr.offRequest(playerXhrRequestHook2); }); +QUnit.test('xhr-hooks-ready event fires as expected', function(assert) { + const done = assert.async(); + + this.player.on('xhr-hooks-ready', (event) => { + assert.equal(event.type, 'xhr-hooks-ready', 'event type is xhr-hooks-ready'); + assert.ok(this.player.tech(true).vhs.xhr.onRequest); + assert.ok(this.player.tech(true).vhs.xhr.onResponse); + assert.ok(this.player.tech(true).vhs.xhr.offRequest); + assert.ok(this.player.tech(true).vhs.xhr.offResponse); + done(); + }); + + this.player.src({ + src: 'main.m3u8', + type: 'application/vnd.apple.mpegurl' + }); + + this.clock.tick(1); +}); + QUnit.test( 'passes useCueTags vhs option to main playlist controller', function(assert) { From 84618bd449ad9b86710f037de440339bd9817578 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 8 May 2023 11:22:15 -0700 Subject: [PATCH 05/14] fix doc typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index cad64378d..1425589a3 100644 --- a/README.md +++ b/README.md @@ -823,7 +823,7 @@ until playback if video.js's `metadata` setting is `none` #### xhr-hooks-ready -Fired when the player `xhr` object is ready to set `onRequest` and `offRequest` hooks, as well +Fired when the player `xhr` object is ready to set `onRequest` and `onResponse` hooks, as well as remove hooks with `offRequest` and `offResponse`. ### VHS Usage Events From 8a30a0ccd328c787352e8b24e058816511eed7cc Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 8 May 2023 14:33:45 -0700 Subject: [PATCH 06/14] fix docs, beforeSend example --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 1425589a3..bec0a079f 100644 --- a/README.md +++ b/README.md @@ -667,7 +667,7 @@ Example: ```javascript const playerXhrRequestHook = (options) => { options.beforeSend = (xhr) => { - xhr.open('GET', 'https://new.uri'); + xhr.setRequestHeader('foo', 'bar'); }; }; player.tech().vhs.xhr.onRequest(playerXhrRequestHook); From ac4ff45b9a52a9413c9b158072fa6772ffb2097b Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 8 May 2023 14:36:35 -0700 Subject: [PATCH 07/14] fix docs, global beforeSend example --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index bec0a079f..e078889fc 100644 --- a/README.md +++ b/README.md @@ -739,7 +739,7 @@ videojs.Vhs.xhr.onRequest(globalRequestHook); // Global request callback defining beforeSend function, will affect every player. const globalXhrRequestHook = (options) => { options.beforeSend = (xhr) => { - xhr.open('GET', 'https://new.uri'); + xhr.setRequestHeader('foo', 'bar'); }; }; videojs.Vhs.xhr.onRequest(globalXhrRequestHook); From c25a03a271c4974e560cf8d37e5a654571754fe7 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 8 May 2023 20:32:55 -0700 Subject: [PATCH 08/14] deprecate beforeRequest and fix tests --- src/xhr.js | 18 +++++++++++------- test/videojs-http-streaming.test.js | 3 +++ test/xhr.test.js | 5 +++++ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/xhr.js b/src/xhr.js index 30b09d741..d60d56d22 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -65,7 +65,7 @@ const callbackWrapper = function(request, error, response, callback) { * @param {Object} response the xhr response object */ const callAllHooks = (hooks, request, error, response) => { - if (!hooks) { + if (!hooks || !hooks.size) { return; } hooks.forEach((hookCallback) => { @@ -82,17 +82,16 @@ const xhrFactory = function() { // Allow an optional user-specified function to modify the option // object before we construct the xhr request + // TODO: Remove beforeRequest in the next major release. const beforeRequest = XhrFunction.beforeRequest || videojs.Vhs.xhr.beforeRequest; // onRequest and onResponse hooks as a Set, at either the player or global level. - const _requestCallbackSet = XhrFunction._requestCallbackSet || videojs.Vhs.xhr._requestCallbackSet; + // TODO: new Set added here for beforeRequest alias. Remove this when beforeRequest is removed. + const _requestCallbackSet = XhrFunction._requestCallbackSet || videojs.Vhs.xhr._requestCallbackSet || new Set(); const _responseCallbackSet = XhrFunction._responseCallbackSet || videojs.Vhs.xhr._responseCallbackSet; if (beforeRequest && typeof beforeRequest === 'function') { - const newOptions = beforeRequest(options); - - if (newOptions) { - options = newOptions; - } + videojs.log.warn('beforeRequest is deprecated, use onRequest instead.'); + _requestCallbackSet.add(beforeRequest); } // Use the standard videojs.xhr() method unless `videojs.Vhs.xhr` has been overriden @@ -101,6 +100,11 @@ const xhrFactory = function() { // call all registered onRequest hooks callAllHooks(_requestCallbackSet, options); + + // Remove the beforeRequest function from the hooks set so stale beforeRequest functions are not called. + _requestCallbackSet.delete(beforeRequest); + + // xhrMethod will call XMLHttpRequest.open and XMLHttpRequest.send const request = xhrMethod(options, function(error, response) { // call all registered onResponse hooks callAllHooks(_responseCallbackSet, request, error, response); diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 9570188e1..88d4b023e 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4005,6 +4005,7 @@ QUnit.test( this.standardXHRResponse(this.requests.shift()); assert.ok(beforeRequestCalled, 'beforeRequest was called'); + assert.equal(this.env.log.warn.calls, 2, 'warning logged for deprecation'); // verify stats assert.equal(this.player.tech_.vhs.stats.bandwidth, 4194304, 'default'); @@ -4030,6 +4031,7 @@ QUnit.test('Allows specifying the beforeRequest function globally', function(ass this.standardXHRResponse(this.requests.shift()); assert.ok(beforeRequestCalled, 'beforeRequest was called'); + assert.equal(this.env.log.warn.calls, 2, 'warning logged for deprecation'); delete videojs.Vhs.xhr.beforeRequest; @@ -4098,6 +4100,7 @@ QUnit.test('Allows overriding the global beforeRequest function', function(asser 'for the media playlist and media'); assert.equal(beforeGlobalRequestCalled, 1, 'global beforeRequest was called once ' + 'for the main playlist'); + assert.equal(this.env.log.warn.calls, 3, 'warning logged for deprecation'); delete videojs.Vhs.xhr.beforeRequest; }); diff --git a/test/xhr.test.js b/test/xhr.test.js index 7ab09f271..9d4025bc9 100644 --- a/test/xhr.test.js +++ b/test/xhr.test.js @@ -33,6 +33,7 @@ QUnit.test('xhr respects beforeRequest', function(assert) { this.xhr(defaultOptions); assert.equal(this.requests.shift().url, 'player', 'url changed with player override'); + assert.equal(this.env.log.warn.calls, 1, 'warning logged for deprecation'); videojs.Vhs.xhr.beforeRequest = (options) => { options.url = 'global'; @@ -41,11 +42,15 @@ QUnit.test('xhr respects beforeRequest', function(assert) { this.xhr(defaultOptions); assert.equal(this.requests.shift().url, 'player', 'prioritizes player override'); + assert.equal(this.env.log.warn.calls, 1, 'warning logged for deprecation'); delete this.xhr.beforeRequest; this.xhr(defaultOptions); assert.equal(this.requests.shift().url, 'global', 'url changed with global override'); + assert.equal(this.env.log.warn.calls, 1, 'warning logged for deprecation'); + + delete videojs.Vhs.xhr.beforeRequest; }); QUnit.test('calls global and player onRequest hooks respectively', function(assert) { From e562b5186d6785b47a6e9d0c28fd58db08c7ae7e Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Mon, 8 May 2023 20:41:53 -0700 Subject: [PATCH 09/14] fix docs, remove beforeRequest --- README.md | 38 ++------------------------------------ 1 file changed, 2 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index e078889fc..cf095f595 100644 --- a/README.md +++ b/README.md @@ -701,25 +701,9 @@ Example: ```javascript player.tech().vhs.xhr.offResponse(playerResponseHook); ``` -Additionally a `beforeRequest` function can be defined, -that will be called with an object containing the options that will be used -to create the xhr request. -Note: any registered `onRequest` hooks, are called _after_ the `beforeRequest` function, so xhr -options modified by this function may be further modified by these hooks. - -Example: -```javascript -player.tech().vhs.xhr.beforeRequest = function(options) { - options.uri = options.uri.replace('example.com', 'foo.com'); - - return options; -}; -``` - -The global `videojs.Vhs` also exposes an `xhr` property. Adding -`onRequest`, `onResponse` hooks and/or specifying a `beforeRequest` -function that will allow you to intercept the request options, xhr +The global `videojs.Vhs` also exposes an `xhr` property. Adding `onRequest` +and/or `onResponse` hooks will allow you to intercept the request options, xhr Object, error and response data for *all* requests in every player on a page. For consistency across browsers the video source should be set at runtime once the video player is ready. @@ -764,24 +748,6 @@ videojs.Vhs.xhr.offRequest(globalRequestHook); videojs.Vhs.xhr.offResponse(globalResponseHook); ``` -```javascript -videojs.Vhs.xhr.beforeRequest = function(options) { - /* - * Modifications to requests that will affect every player. - */ - - return options; -}; - -var player = videojs('video-player-id'); -player.ready(function() { - this.src({ - src: 'https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/bipbop_16x9_variant.m3u8', - type: 'application/x-mpegURL', - }); -}); -``` - For information on the type of options that you can modify see the documentation at [https://github.com/Raynos/xhr](https://github.com/Raynos/xhr). From 27728762223802ccb12213d3814a6f573ea8ee75 Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Tue, 9 May 2023 15:13:26 -0700 Subject: [PATCH 10/14] add support for callback return options --- src/xhr.js | 20 +++++++++++++------- test/xhr.test.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/xhr.js b/src/xhr.js index d60d56d22..ccfc97e2b 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -60,17 +60,23 @@ const callbackWrapper = function(request, error, response, callback) { * Iterates over a Set of callback hooks and calls them in order * * @param {Set} hooks the hook set to iterate over - * @param {Object} request the xhr request object - * @param {Object} error the xhr error object - * @param {Object} response the xhr response object + * @param {Object} request the request options or object + * @param {Object} error the error object + * @param {Object} response the response object + * + * @return the callback hook function return value. */ const callAllHooks = (hooks, request, error, response) => { if (!hooks || !hooks.size) { return; } + let hookOptions; + hooks.forEach((hookCallback) => { - hookCallback(request, error, response); + // Pass the returned options back to the next hook callback to support a callback returning a new options object. + hookOptions = hookCallback(hookOptions || request, error, response); }); + return hookOptions; }; const xhrFactory = function() { @@ -98,14 +104,14 @@ const xhrFactory = function() { // TODO: switch back to videojs.Vhs.xhr.name === 'XhrFunction' when we drop IE11 const xhrMethod = videojs.Vhs.xhr.original === true ? videojsXHR : videojs.Vhs.xhr; - // call all registered onRequest hooks - callAllHooks(_requestCallbackSet, options); + // call all registered onRequest hooks, options returned for beforeRequest support. + const beforeRequestOptions = callAllHooks(_requestCallbackSet, options); // Remove the beforeRequest function from the hooks set so stale beforeRequest functions are not called. _requestCallbackSet.delete(beforeRequest); // xhrMethod will call XMLHttpRequest.open and XMLHttpRequest.send - const request = xhrMethod(options, function(error, response) { + const request = xhrMethod(beforeRequestOptions || options, function(error, response) { // call all registered onResponse hooks callAllHooks(_responseCallbackSet, request, error, response); return callbackWrapper(request, error, response, callback); diff --git a/test/xhr.test.js b/test/xhr.test.js index 9d4025bc9..ee7727b90 100644 --- a/test/xhr.test.js +++ b/test/xhr.test.js @@ -53,6 +53,37 @@ QUnit.test('xhr respects beforeRequest', function(assert) { delete videojs.Vhs.xhr.beforeRequest; }); +QUnit.test('beforeRequest can return a new options object', function(assert) { + const defaultOptions = { + url: 'default' + }; + + this.xhr(defaultOptions); + assert.equal(this.requests.shift().url, 'default', 'url the same without override'); + + videojs.Vhs.xhr.beforeRequest = () => { + return { uri: 'global-newOptions'}; + }; + + this.xhr(defaultOptions); + assert.equal(this.requests.shift().url, 'global-newOptions', 'url changed with global override'); + assert.equal(this.env.log.warn.calls, 1, 'warning logged for deprecation'); + + this.xhr.beforeRequest = () => { + return { uri: 'player-newOptions'}; + }; + + this.xhr(defaultOptions); + assert.equal(this.requests.shift().url, 'player-newOptions', 'url changed with player override'); + assert.equal(this.env.log.warn.calls, 1, 'warning logged for deprecation'); + + delete this.xhr.beforeRequest; + delete videojs.Vhs.xhr.beforeRequest; + + this.xhr(defaultOptions); + assert.equal(this.requests.shift().url, 'default', 'url the same without override'); +}); + QUnit.test('calls global and player onRequest hooks respectively', function(assert) { const defaultOptions = { url: 'default' From f1dadd890e01f047125e4496bb59a65d6deb063d Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Tue, 9 May 2023 15:16:35 -0700 Subject: [PATCH 11/14] fix comment style --- src/videojs-http-streaming.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/videojs-http-streaming.js b/src/videojs-http-streaming.js index add7b6532..38cb53c0c 100644 --- a/src/videojs-http-streaming.js +++ b/src/videojs-http-streaming.js @@ -1333,10 +1333,8 @@ class VhsHandler extends Component { removeOnResponseHook(this.xhr, callback); }; - /** - * Trigger an event on the player to notify the user that vhs is ready to set xhr hooks. - * This allows hooks to be set before the source is set to vhs when handleSource is called. - */ + // Trigger an event on the player to notify the user that vhs is ready to set xhr hooks. + // This allows hooks to be set before the source is set to vhs when handleSource is called. this.player_.trigger('xhr-hooks-ready'); } } From 4e3130f24a7bc15750b2ab4f84505206a1ada5fc Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Wed, 10 May 2023 13:02:21 -0700 Subject: [PATCH 12/14] docs update, add return values and event --- README.md | 52 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index cf095f595..b51e60a4b 100644 --- a/README.md +++ b/README.md @@ -640,21 +640,36 @@ Type: `function` The xhr function that is used by VHS internally is exposed on the per- player `vhs` object. While it is possible, we do not recommend replacing the function with your own implementation. Instead, `xhr` provides -the ability to specify `onRequest` and `onResponse` hooks which take a -callback function as a parameter as well as `offRequest` and `offResponse` -functions which will remove a callback function from the `onRequest` or -`onResponse` set if it exists. +the ability to specify `onRequest` and `onResponse` hooks which each take a +callback function as a parameter, as well as `offRequest` and `offResponse` +functions which can remove a callback function from the `onRequest` or +`onResponse` Set. -The `onRequest(callback)` function takes a `callback` function that will pass the xhr `options` +Additionally, an `xhr-hooks-ready` event is fired from a player when +per-player hooks are ready to be added or removed. This will ensure player +specific hooks are set prior to any manifest or segment requests. + +Example: +```javascript +player.on('xhr-hooks-ready', () => { + const playerRequestHook = (options) => options; + player.tech().vhs.xhr.onRequest(playerRequestHook); +}); +``` + +The `onRequest(callback)` function takes a `callback` function that will pass an xhr `options` Object to that callback. These callbacks are called synchronously, in the order registered and act as pre-request hooks for modifying the xhr `options` Object prior to making a request. +Note: This callback *MUST* return an `options` Object as the `xhr` wrapper and each `onRequest` +hook receives the returned `options` as a parameter. + Example: ```javascript const playerRequestHook = (options) => { - const requestUrl = new URL(options.uri); - requestUrl.searchParams.set('foo', 'bar'); - options.uri = requestUrl.href; + return { + uri: 'https://new.options.uri' + }; }; player.tech().vhs.xhr.onRequest(playerRequestHook); ``` @@ -669,6 +684,7 @@ const playerXhrRequestHook = (options) => { options.beforeSend = (xhr) => { xhr.setRequestHeader('foo', 'bar'); }; + return options; }; player.tech().vhs.xhr.onRequest(playerXhrRequestHook); ``` @@ -676,7 +692,8 @@ player.tech().vhs.xhr.onRequest(playerXhrRequestHook); The `onResponse(callback)` function takes a `callback` function that will pass the xhr `request`, `error`, and `response` Objects to that callback. These callbacks are called in the order registered and act as post-request hooks for gathering data from the -xhr `request`, `error` and `response` Objects. +xhr `request`, `error` and `response` Objects. `onResponse` callbacks do not require a +return value, the parameters are passed to each subsequent callback by reference. Example: ```javascript @@ -702,19 +719,19 @@ Example: player.tech().vhs.xhr.offResponse(playerResponseHook); ``` -The global `videojs.Vhs` also exposes an `xhr` property. Adding `onRequest` -and/or `onResponse` hooks will allow you to intercept the request options, xhr -Object, error and response data for *all* requests in every player on a page. -For consistency across browsers the video source should be set at runtime -once the video player is ready. +The global `videojs.Vhs` also exposes an `xhr` property. Adding `onRequest` +and/or `onResponse` hooks will allow you to intercept the request options and xhr +Object as well as request, error, and response data for *all* requests in *every* +player on a page. For consistency across browsers the video source should be set +at runtime once the video player is ready. Example: ```javascript // Global request callback, will affect every player. const globalRequestHook = (options) => { - const requestUrl = new URL(options.uri); - requestUrl.searchParams.set('foo', 'bar'); - options.uri = requestUrl.href; + return { + uri: 'https://new.options.global.uri' + }; }; videojs.Vhs.xhr.onRequest(globalRequestHook); ``` @@ -725,6 +742,7 @@ const globalXhrRequestHook = (options) => { options.beforeSend = (xhr) => { xhr.setRequestHeader('foo', 'bar'); }; + return options; }; videojs.Vhs.xhr.onRequest(globalXhrRequestHook); ``` From 538aac2398667664e8fc54e70d8e24fcf776049f Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Wed, 10 May 2023 15:13:59 -0700 Subject: [PATCH 13/14] fix readme, add event listener to all examples --- README.md | 60 +++++++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index b51e60a4b..d581d4958 100644 --- a/README.md +++ b/README.md @@ -643,19 +643,9 @@ the function with your own implementation. Instead, `xhr` provides the ability to specify `onRequest` and `onResponse` hooks which each take a callback function as a parameter, as well as `offRequest` and `offResponse` functions which can remove a callback function from the `onRequest` or -`onResponse` Set. - -Additionally, an `xhr-hooks-ready` event is fired from a player when -per-player hooks are ready to be added or removed. This will ensure player -specific hooks are set prior to any manifest or segment requests. - -Example: -```javascript -player.on('xhr-hooks-ready', () => { - const playerRequestHook = (options) => options; - player.tech().vhs.xhr.onRequest(playerRequestHook); -}); -``` +`onResponse` Set. An `xhr-hooks-ready` event is fired from a player when per-player +hooks are ready to be added or removed. This will ensure player specific hooks are +set prior to any manifest or segment requests. The `onRequest(callback)` function takes a `callback` function that will pass an xhr `options` Object to that callback. These callbacks are called synchronously, in the order registered @@ -666,12 +656,14 @@ hook receives the returned `options` as a parameter. Example: ```javascript -const playerRequestHook = (options) => { - return { - uri: 'https://new.options.uri' +player.on('xhr-hooks-ready', () => { + const playerRequestHook = (options) => { + return { + uri: 'https://new.options.uri' + }; }; -}; -player.tech().vhs.xhr.onRequest(playerRequestHook); + player.tech().vhs.xhr.onRequest(playerRequestHook); +}); ``` If access to the `xhr` Object is required prior to the `xhr.send` call, an `options.beforeSend` @@ -680,13 +672,15 @@ as a parameter and will be called immediately prior to `xhr.send`. Example: ```javascript -const playerXhrRequestHook = (options) => { - options.beforeSend = (xhr) => { - xhr.setRequestHeader('foo', 'bar'); +player.on('xhr-hooks-ready', () => { + const playerXhrRequestHook = (options) => { + options.beforeSend = (xhr) => { + xhr.setRequestHeader('foo', 'bar'); + }; + return options; }; - return options; -}; -player.tech().vhs.xhr.onRequest(playerXhrRequestHook); + player.tech().vhs.xhr.onRequest(playerXhrRequestHook); +}); ``` The `onResponse(callback)` function takes a `callback` function that will pass the xhr @@ -697,10 +691,12 @@ return value, the parameters are passed to each subsequent callback by reference Example: ```javascript -const playerResponseHook = (request, error, response) => { - const bar = response.headers.foo -}; -player.tech().vhs.xhr.onResponse(playerResponseHook); +player.on('xhr-hooks-ready', () => { + const playerResponseHook = (request, error, response) => { + const bar = response.headers.foo; + }; + player.tech().vhs.xhr.onResponse(playerResponseHook); +}); ``` The `offRequest` function takes a `callback` function, and will remove that function from @@ -708,7 +704,9 @@ the collection of `onRequest` hooks if it exists. Example: ```javascript -player.tech().vhs.xhr.offRequest(playerRequestHook); +player.on('xhr-hooks-ready', () => { + player.tech().vhs.xhr.offRequest(playerRequestHook); +}); ``` The `offResponse` function takes a `callback` function, and will remove that function from @@ -716,7 +714,9 @@ the collection of `offResponse` hooks if it exists. Example: ```javascript -player.tech().vhs.xhr.offResponse(playerResponseHook); +player.on('xhr-hooks-ready', () => { + player.tech().vhs.xhr.offResponse(playerResponseHook); +}); ``` The global `videojs.Vhs` also exposes an `xhr` property. Adding `onRequest` From 7f062e9d1ea0e5281d7ddcd045cb44153fb7bc4f Mon Sep 17 00:00:00 2001 From: Adam Waldron Date: Wed, 10 May 2023 15:33:22 -0700 Subject: [PATCH 14/14] split callAllHooks into 2 functions and fix tests --- src/xhr.js | 47 ++++++++++++++++++----------- test/videojs-http-streaming.test.js | 24 ++++++++++++--- test/xhr.test.js | 20 +++++++----- 3 files changed, 61 insertions(+), 30 deletions(-) diff --git a/src/xhr.js b/src/xhr.js index ccfc97e2b..7d3c213d8 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -57,26 +57,39 @@ const callbackWrapper = function(request, error, response, callback) { }; /** - * Iterates over a Set of callback hooks and calls them in order + * Iterates over the request hooks Set and calls them in order * - * @param {Set} hooks the hook set to iterate over - * @param {Object} request the request options or object - * @param {Object} error the error object - * @param {Object} response the response object - * - * @return the callback hook function return value. + * @param {Set} hooks the hook Set to iterate over + * @param {Object} options the request options to pass to the xhr wrapper + * @return the callback hook function return value, the modified or new options Object. */ -const callAllHooks = (hooks, request, error, response) => { - if (!hooks || !hooks.size) { +const callAllRequestHooks = (requestSet, options) => { + if (!requestSet || !requestSet.size) { return; } - let hookOptions; + let newOptions = options; - hooks.forEach((hookCallback) => { - // Pass the returned options back to the next hook callback to support a callback returning a new options object. - hookOptions = hookCallback(hookOptions || request, error, response); + requestSet.forEach((requestCallback) => { + newOptions = requestCallback(newOptions); + }); + return newOptions; +}; + +/** + * Iterates over the response hooks Set and calls them in order. + * + * @param {Set} hooks the hook Set to iterate over + * @param {Object} request the xhr request object + * @param {Object} error the xhr error object + * @param {Object} response the xhr response object + */ +const callAllResponseHooks = (responseSet, request, error, response) => { + if (!responseSet || !responseSet.size) { + return; + } + responseSet.forEach((responseCallback) => { + responseCallback(request, error, response); }); - return hookOptions; }; const xhrFactory = function() { @@ -104,8 +117,8 @@ const xhrFactory = function() { // TODO: switch back to videojs.Vhs.xhr.name === 'XhrFunction' when we drop IE11 const xhrMethod = videojs.Vhs.xhr.original === true ? videojsXHR : videojs.Vhs.xhr; - // call all registered onRequest hooks, options returned for beforeRequest support. - const beforeRequestOptions = callAllHooks(_requestCallbackSet, options); + // call all registered onRequest hooks, assign new options. + const beforeRequestOptions = callAllRequestHooks(_requestCallbackSet, options); // Remove the beforeRequest function from the hooks set so stale beforeRequest functions are not called. _requestCallbackSet.delete(beforeRequest); @@ -113,7 +126,7 @@ const xhrFactory = function() { // xhrMethod will call XMLHttpRequest.open and XMLHttpRequest.send const request = xhrMethod(beforeRequestOptions || options, function(error, response) { // call all registered onResponse hooks - callAllHooks(_responseCallbackSet, request, error, response); + callAllResponseHooks(_responseCallbackSet, request, error, response); return callbackWrapper(request, error, response, callback); }); const originalAbort = request.abort; diff --git a/test/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 88d4b023e..7be0ad38e 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4123,9 +4123,11 @@ QUnit.test('Allows setting onRequest hooks globally', function(assert) { requestUrl.searchParams.set('foo', 'bar'); actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; + return options; }; - const globalRequestHook2 = () => { + const globalRequestHook2 = (options) => { onRequestHookCallCount++; + return options; }; videojs.Vhs.xhr.onRequest(globalRequestHook1); @@ -4159,9 +4161,11 @@ QUnit.test('Allows setting onRequest hooks on the player', function(assert) { requestUrl.searchParams.set('foo', 'bar'); actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; + return options; }; - const playerRequestHook2 = () => { + const playerRequestHook2 = (options) => { onRequestHookCallCount++; + return options; }; // Setup player level xhr hooks. @@ -4200,9 +4204,11 @@ QUnit.test('Allows setting onRequest hooks globally and overriding with player h requestUrl.searchParams.set('foo', 'bar'); actualRequestUrlGlobal = options.uri = requestUrl.href; onRequestHookCallCountGlobal++; + return options; }; - const globalRequestHook2 = () => { + const globalRequestHook2 = (options) => { onRequestHookCallCountGlobal++; + return options; }; videojs.Vhs.xhr.onRequest(globalRequestHook1); @@ -4214,9 +4220,11 @@ QUnit.test('Allows setting onRequest hooks globally and overriding with player h requestUrl.searchParams.set('bar', 'foo'); actualRequestUrlPlayer = options.uri = requestUrl.href; onRequestHookCallCountPlayer++; + return options; }; - const playerRequestHook2 = () => { + const playerRequestHook2 = (options) => { onRequestHookCallCountPlayer++; + return options; }; // Setup player level xhr hooks. @@ -4264,9 +4272,11 @@ QUnit.test('Allows removing onRequest hooks globally with offRequest', function( requestUrl.searchParams.set('foo', 'bar'); actualRequestUrl = options.uri = requestUrl.href; onRequestHookCallCount++; + return options; }; - const globalRequestHook2 = () => { + const globalRequestHook2 = (options) => { onRequestHookCallCount++; + return options; }; videojs.Vhs.xhr.onRequest(globalRequestHook1); @@ -4462,6 +4472,7 @@ QUnit.test('Allows xhr object access in global onRequest hooks', function(assert xhr.open('GET', 'https://foo.bar'); }; onRequestHookCallCount++; + return options; }; const globalXhrRequestHook2 = (options) => { options.beforeSend = (xhr) => { @@ -4469,6 +4480,7 @@ QUnit.test('Allows xhr object access in global onRequest hooks', function(assert expectedUrl = xhr.url; }; onRequestHookCallCount++; + return options; }; videojs.Vhs.xhr.onRequest(globalXhrRequestHook1); @@ -4501,6 +4513,7 @@ QUnit.test('Allows xhr object access in player onRequest hooks', function(assert xhr.open('GET', 'https://new.url'); }; onRequestHookCallCount++; + return options; }; const playerXhrRequestHook2 = (options) => { options.beforeSend = (xhr) => { @@ -4508,6 +4521,7 @@ QUnit.test('Allows xhr object access in player onRequest hooks', function(assert expectedUrl = xhr.url; }; onRequestHookCallCount++; + return options; }; // Setup player level xhr hooks. diff --git a/test/xhr.test.js b/test/xhr.test.js index ee7727b90..83cace694 100644 --- a/test/xhr.test.js +++ b/test/xhr.test.js @@ -94,13 +94,15 @@ QUnit.test('calls global and player onRequest hooks respectively', function(asse // create the global onRequest set and 2 hooks videojs.Vhs.xhr._requestCallbackSet = new Set(); - const globalRequestHook1 = (request) => { - request.url = 'global'; + const globalRequestHook1 = (options) => { + options.url = 'global'; + return options; }; - const globalRequestHook2 = (request) => { - request.headers = { + const globalRequestHook2 = (options) => { + options.headers = { foo: 'bar' }; + return options; }; // add them to the set @@ -115,13 +117,15 @@ QUnit.test('calls global and player onRequest hooks respectively', function(asse // create the player onRequest set and 2 hooks this.xhr._requestCallbackSet = new Set(); - const playerRequestHook1 = (request) => { - request.url = 'player'; + const playerRequestHook1 = (options) => { + options.url = 'player'; + return options; }; - const playerRequestHook2 = (request) => { - request.headers = { + const playerRequestHook2 = (options) => { + options.headers = { bar: 'foo' }; + return options; }; // add them to the set