fix: onRequest hooks called too late (#1396)

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)
@@ -639,85 +640,111 @@ 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. 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 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 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 = (request) => {
-  const requestUrl = new URL(request.uri);
-  requestUrl.searchParams.set('foo', 'bar');
-  request.uri = requestUrl.href;
-};
-player.tech().vhs.xhr.onRequest(playerRequestHook);
+player.on('xhr-hooks-ready', () => {
+  const playerRequestHook = (options) => {
+    return {
+      uri: 'https://new.options.uri'
+    };
+  };
+  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
+player.on('xhr-hooks-ready', () => {
+  const playerXhrRequestHook = (options) => {
+    options.beforeSend = (xhr) => {
+      xhr.setRequestHeader('foo', 'bar');
+    };
+    return options;
+  };
+  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
-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
 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
 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);
+});
 ```
-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.
+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
-player.tech().vhs.xhr.beforeRequest = function(options) {
-  options.uri = options.uri.replace('example.com', 'foo.com');
-
-  return options;
+// Global request callback, will affect every player.
+const globalRequestHook = (options) => {
+  return {
+    uri: 'https://new.options.global.uri'
+  };
 };
+videojs.Vhs.xhr.onRequest(globalRequestHook);
 ```
 
-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 
-once the video player is ready.
-
-Example:
 ```javascript
-// Global request callback, will affect every player.
-const globalRequestHook = (request) => {
-  const requestUrl = new URL(request.uri);
-  requestUrl.searchParams.set('foo', 'bar');
-  request.uri = requestUrl.href;
+// Global request callback defining beforeSend function, will affect every player.
+const globalXhrRequestHook = (options) => {
+  options.beforeSend = (xhr) => {
+    xhr.setRequestHeader('foo', 'bar');
+  };
+  return options;
 };
-videojs.Vhs.xhr.onRequest(globalRequestHook);
+videojs.Vhs.xhr.onRequest(globalXhrRequestHook);
 ```
 
 ```javascript
@@ -739,24 +766,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).
 
@@ -796,6 +805,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 `onResponse` 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,

src/videojs-http-streaming.js

@@ -1332,6 +1332,10 @@ 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 +1360,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;
   },

src/xhr.js

@@ -57,19 +57,38 @@ 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 {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 callAllRequestHooks = (requestSet, options) => {
+  if (!requestSet || !requestSet.size) {
+    return;
+  }
+  let newOptions = options;
+
+  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 callAllHooks = (hooks, request, error, response) => {
-  if (!hooks) {
+const callAllResponseHooks = (responseSet, request, error, response) => {
+  if (!responseSet || !responseSet.size) {
     return;
   }
-  hooks.forEach((hookCallback) => {
-    hookCallback(request, error, response);
+  responseSet.forEach((responseCallback) => {
+    responseCallback(request, error, response);
   });
 };
 
@@ -82,26 +101,32 @@ 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
     // TODO: switch back to videojs.Vhs.xhr.name === 'XhrFunction' when we drop IE11
     const xhrMethod = videojs.Vhs.xhr.original === true ? videojsXHR : videojs.Vhs.xhr;
 
-    const request = xhrMethod(options, function(error, response) {
+    // 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);
+
+    // 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;
@@ -112,9 +137,6 @@ const xhrFactory = function() {
     };
     request.uri = options.uri;
     request.requestTime = Date.now();
-    // call all registered onRequest hooks
-    callAllHooks(_requestCallbackSet, request);
-
     return request;
   };