Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: onRequest hooks called too late #1396

Merged
merged 14 commits into from
May 15, 2023
Merged

fix: onRequest hooks called too late #1396

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3340014
fix: onRequest hooks called too late
adrums86 May 5, 2023
cdfa622
fix test hooks and docs
adrums86 May 5, 2023
66865b8
add xhr-hooks-ready event
adrums86 May 8, 2023
1646e2d
add event test
adrums86 May 8, 2023
84618bd
fix doc typo
adrums86 May 8, 2023
8a30a0c
fix docs, beforeSend example
adrums86 May 8, 2023
ac4ff45
fix docs, global beforeSend example
adrums86 May 8, 2023
c25a03a
deprecate beforeRequest and fix tests
adrums86 May 9, 2023
e562b51
fix docs, remove beforeRequest
adrums86 May 9, 2023
2772876
add support for callback return options
adrums86 May 9, 2023
f1dadd8
fix comment style
adrums86 May 9, 2023
4e3130f
docs update, add return values and event
adrums86 May 10, 2023
538aac2
fix readme, add event listener to all examples
adrums86 May 10, 2023
7f062e9
split callAllHooks into 2 functions and fix tests
adrums86 May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 42 additions & 12 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -644,20 +645,34 @@ 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.

Example:
```javascript
const playerRequestHook = (request) => {
const requestUrl = new URL(request.uri);
const playerRequestHook = (options) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought on these samples, should they be wrapped in an xhr-hooks-ready event callback to show the recommended approach?

player.on('xhr-hooks-ready', () => {
  player.tech().vhs.xhr.onRequest((options) => options);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, will add examples with the event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the 'xhr-hooks-ready' event listener to all the per-player examples.

const requestUrl = new URL(options.uri);
requestUrl.searchParams.set('foo', 'bar');
request.uri = requestUrl.href;
options.uri = requestUrl.href;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to update these samples to return the options object (or a new object) in the same pattern as beforeRequest.

It would make sense to document the behavior here - each onRequest hook should return an options object and each will receive the previous hook's options object.

Also, we should document that the onResponse hooks do not behave this way - the request object is passed to each by reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't 100% sure that we wanted to document it that way as it wasn't clear whether we would remove the options return/chaining after beforeRequest is removed. I agree it makes sense if we keep it that way for onRequest hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added options return to the onRequest examples. Documented onResponse passing parameters by reference as well.

};
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
Expand Down Expand Up @@ -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');
adrums86 marked this conversation as resolved.
Show resolved Hide resolved
};
};
videojs.Vhs.xhr.onRequest(globalXhrRequestHook);
```

```javascript
// Global response hook callback, will affect every player.
const globalResponseHook = (request, error, response) => {
Expand Down Expand Up @@ -796,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,
Expand Down
7 changes: 6 additions & 1 deletion src/videojs-http-streaming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
adrums86 marked this conversation as resolved.
Show resolved Hide resolved
this.player_.trigger('xhr-hooks-ready');
}
}

Expand All @@ -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;
},
Expand Down
5 changes: 2 additions & 3 deletions src/xhr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like options is passed here but no return value is used, which would be a change from the above removed behavior where beforeRequest returned a new options object. It seems that this new expectation is that each request callback would reference the same options object - each potentially modifying the same object in sequence.

I don't think this is a bad implementation, but it could be a breaking change to beforeRequest. Consider this would likely fail with this refactor:

videojs.Vhs.xhr.beforeRequest = (options) => {
  // Ignores `options` and returns a totally new object.
  return {...};
};

We probably don't want that to break, so perhaps the callAllHooks should have special handling for the beforeRequest/onRequest hooks where it passes the return value of the previous hook through a chain and returns the last result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch @misteroneill ! I added some logic to support both cases and a test for the use case you described. Let me know what you think.

const request = xhrMethod(options, function(error, response) {
// call all registered onResponse hooks
callAllHooks(_responseCallbackSet, request, error, response);
Expand All @@ -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;
};

Expand Down
136 changes: 116 additions & 20 deletions test/videojs-http-streaming.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -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 = () => {
Expand All @@ -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 = () => {
Expand Down Expand Up @@ -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 = () => {
Expand Down Expand Up @@ -4447,6 +4442,107 @@ 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('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) {
Expand Down