fix: onRequest hooks called too late #1396

adrums86 · 2023-05-05T15:47:46Z

Description

The xhr wrapper calls open and send when a request object is instantiated, this is causing onRequest hooks to modify the xhr object too late, as they were already sent when the object was returned to the VHS xhr module. Additionally, setting xhr hooks in time for the initial manifest request when preload is set to auto or metadata is not possible with existing events or ready callbacks.

Specific Changes proposed

Moving the request hooks callback and changing the callback parameter

By calling all the onRequest hooks before we create the request object and passing the callback the options object instead, we can modify the request before open is called in the xhr wrapper. This options object allows for modifying the url/headers/methods, as well as setting a beforeSend callback, which gives the onRequest callback direct access to the xhr object via a nested callback. beforeSend will be called immediately before xhr.send is called, which allows for full control of the xhr object prior to sending the request.

For example, setting a request header.

const playerXhrRequestHook = (options) => {
  options.beforeSend = (xhr) => {
    xhr.setRequestHeader('foo', 'bar');
  };
};
player.tech().vhs.xhr.onRequest(playerXhrRequestHook);

See all supported options: https://github.com/videojs/xhr#options

Adding an xhr-hooks-ready event

This fixes a preload option issue in VHS, where if metadata is preloaded setting onRequest hooks is not possible on player.ready because player.tech().vhs doesn't exist yet as it's created in handleSources. Also if the user tries to set onRequest hooks on the loadstart event, it's too late and the callback will not be set before the manifest is requested. Thus, a new event is needed to be fired immediately when the hooks are available, which allows the user to apply these hooks to all requests, including the main manifest.

Deprecating xhr.beforeRequest

Since adding an onRequest hooks is functionally the same as setting a xhr.beforeRequest callback, a deprecation warning will log when setting a beforeRequest function. beforeRequest will also now alias to an onRequest callback.

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

codecov · 2023-05-05T17:35:44Z

Codecov Report

Merging #1396 (7f062e9) into main (d28a33b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1396      +/-   ##
==========================================
+ Coverage   85.46%   85.47%   +0.01%     
==========================================
  Files          40       40              
  Lines       10017    10025       +8     
  Branches     2319     2320       +1     
==========================================
+ Hits         8561     8569       +8     
  Misses       1456     1456

Impacted Files	Coverage Δ
src/videojs-http-streaming.js	`90.98% <100.00%> (+0.01%)`	⬆️
src/xhr.js	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

misteroneill

We've talked about this offline, but it's just now occurring to me... with this change to onRequest, is there functionally any difference between the onRequest hook and the beforeRequest thing that already existed?

adrums86 · 2023-05-08T18:38:23Z

@misteroneill It's definitely a bit of hair splitting at this point, but there seems to be no real functional difference. Especially with the revelation that the options object can pass a beforeSend callback which provides access to the xhr object. We could properly differentiate these by modifying the XHR wrapper, but that is also another can-o-worms and still functionally the same. I think the biggest fix here is the event. beforeRequest has the same timing issue as the hooks API.

adrums86 · 2023-05-08T20:00:22Z

The biggest reason for my decision to have onRequest take options is the way the xhr wrapper works. When the request object is created, XMLHttpRequest.open() is called with the supplied options first, then the options.beforeSend callback is called, then finally XMLHttpRequest.send() is called. Because of this, modifying the request or xhr object URL before send is called requires another call to XMLHttpRequest.open with the modified URL. This seemed a bit clumsy to me, as if you intended to modify every request URL, it would require 2 calls to XMLHttpRequest.open.

gkatsev · 2023-05-08T20:09:07Z

if beforeRequest and onRequest are basically functionality equivalent, we could deprecate beforeRequest to have consistent naming with onRequest and onResponse. Also, completely missed that beforeSend was a thing that could be provided to the options object. We probably could've saved a lot of headache if we realized that ahead of time, haha.

gkatsev

should beforeRequest alias onRequest?

README.md

adrums86 · 2023-05-08T21:00:44Z

Yeah I kind of figured this was headed toward the depreciation and/or aliasing of beforeRequest, for some reason I didn't think to explicitly call that out until you mentioned it. I'll add a warning to beforeRequest and create an alias to onRequest.

adrums86 · 2023-05-09T03:44:12Z

Added a deprecation warning to beforeRequest and aliased the function to an onRequest callback. Also removed beforeRequest from the docs.

src/videojs-http-streaming.js

misteroneill · 2023-05-09T18:27:34Z

src/xhr.js

    }

    // 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;

+    // call all registered onRequest hooks
+    callAllHooks(_requestCallbackSet, options);


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?

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.

adrums86 · 2023-05-09T23:06:15Z

Couple of open question I would appreciate some feedback on per the latest changes.

Is it better to merge or || (or) the options returned from beforeRequest and/or hooks?
Should we handle onRequest and onResponse hooks separately, or still use callAllHooks for both?

misteroneill

Code looks solid to me. I think some documentation updates are all that's needed now.

Is it better to merge or || (or) the options returned from beforeRequest and/or hooks?

I think || is better in this case. Since each onRequest (including beforeRequest) should return an options object, my expectation as a developer would be that videojs.xhr receives the options returned by the last hook function called.

Should we handle onRequest and onResponse hooks separately, or still use callAllHooks for both?

I guess you asked this because of this:

hookOptions = hookCallback(hookOptions || request, error, response);

The only reservation I have about that is that if an onResponse hook returns a new request object, it could replace the original request object on the next onResponse hook.

It may be clearer and cleaner to create separate callAllHooks functions since they would behave slightly differently.

misteroneill · 2023-05-10T12:47:32Z

README.md


 Example:
 ```javascript
-const playerRequestHook = (request) => {
-  const requestUrl = new URL(request.uri);
+const playerRequestHook = (options) => {


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); });

Good call, will add examples with the event.

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

misteroneill · 2023-05-10T12:50:38Z

README.md

  requestUrl.searchParams.set('foo', 'bar');
-  request.uri = requestUrl.href;
+  options.uri = requestUrl.href;


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.

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.

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

adrums86 · 2023-05-10T22:59:46Z

Ended up splitting callAllHooks into 2 functions to better accommodate the requirement for onRequest callbacks to return an options object.

adrums86 added 2 commits May 4, 2023 23:10

fix: onRequest hooks called too late

3340014

fix test hooks and docs

cdfa622

adrums86 marked this pull request as ready for review May 5, 2023 17:48

adrums86 added 2 commits May 7, 2023 21:40

add xhr-hooks-ready event

66865b8

add event test

1646e2d

adrums86 self-assigned this May 8, 2023

adrums86 added the needs: review label May 8, 2023

fix doc typo

84618bd

misteroneill reviewed May 8, 2023

View reviewed changes

gkatsev reviewed May 8, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

adrums86 added 4 commits May 8, 2023 14:33

fix docs, beforeSend example

8a30a0c

fix docs, global beforeSend example

ac4ff45

deprecate beforeRequest and fix tests

c25a03a

fix docs, remove beforeRequest

e562b51

adrums86 requested review from misteroneill and gkatsev May 9, 2023 03:45

misteroneill requested changes May 9, 2023

View reviewed changes

adrums86 added 2 commits May 9, 2023 15:13

add support for callback return options

2772876

fix comment style

f1dadd8

misteroneill requested changes May 10, 2023

View reviewed changes

adrums86 added 3 commits May 10, 2023 13:02

docs update, add return values and event

4e3130f

fix readme, add event listener to all examples

538aac2

split callAllHooks into 2 functions and fix tests

7f062e9

misteroneill approved these changes May 11, 2023

View reviewed changes

adrums86 removed the needs: review label May 15, 2023

adrums86 merged commit 19539ea into main May 15, 2023

adrums86 deleted the fix-onRequest branch May 15, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: onRequest hooks called too late #1396

fix: onRequest hooks called too late #1396

adrums86 commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

misteroneill left a comment •

edited

Loading

adrums86 commented May 8, 2023 •

edited

Loading

adrums86 commented May 8, 2023 •

edited

Loading

gkatsev commented May 8, 2023

gkatsev left a comment

adrums86 commented May 8, 2023 •

edited

Loading

adrums86 commented May 9, 2023

misteroneill May 9, 2023

adrums86 May 9, 2023

adrums86 commented May 9, 2023

misteroneill left a comment

misteroneill May 10, 2023

adrums86 May 10, 2023

adrums86 May 10, 2023

misteroneill May 10, 2023

adrums86 May 10, 2023

adrums86 May 10, 2023

adrums86 commented May 10, 2023

fix: onRequest hooks called too late #1396

fix: onRequest hooks called too late #1396

Conversation

adrums86 commented May 5, 2023 • edited Loading

Description

Specific Changes proposed

Moving the request hooks callback and changing the callback parameter

Adding an xhr-hooks-ready event

Deprecating xhr.beforeRequest

Requirements Checklist

codecov bot commented May 5, 2023 • edited Loading

Codecov Report

misteroneill left a comment • edited Loading

Choose a reason for hiding this comment

adrums86 commented May 8, 2023 • edited Loading

adrums86 commented May 8, 2023 • edited Loading

gkatsev commented May 8, 2023

gkatsev left a comment

Choose a reason for hiding this comment

adrums86 commented May 8, 2023 • edited Loading

adrums86 commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrums86 commented May 9, 2023

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrums86 commented May 10, 2023

adrums86 commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

misteroneill left a comment •

edited

Loading

adrums86 commented May 8, 2023 •

edited

Loading

adrums86 commented May 8, 2023 •

edited

Loading

adrums86 commented May 8, 2023 •

edited

Loading