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.

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

fix: onRequest hooks called too late #1396

Merged
merged 14 commits into from
May 15, 2023
Merged

fix: onRequest hooks called too late #1396

merged 14 commits into from
May 15, 2023

Conversation

adrums86
Copy link
Contributor

@adrums86 adrums86 commented May 5, 2023

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
  • Reviewed by Two Core Contributors

Sorry, something went wrong.

@codecov
Copy link

codecov bot commented May 5, 2023

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

@adrums86 adrums86 marked this pull request as ready for review May 5, 2023 17:48
@adrums86 adrums86 self-assigned this May 8, 2023
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

adrums86 commented May 8, 2023

@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
Copy link
Contributor Author

adrums86 commented May 8, 2023

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
Copy link
Member

gkatsev commented May 8, 2023

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.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

should beforeRequest alias onRequest?

README.md Outdated Show resolved Hide resolved
@adrums86
Copy link
Contributor Author

adrums86 commented May 8, 2023

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
Copy link
Contributor Author

adrums86 commented May 9, 2023

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

@adrums86 adrums86 requested review from misteroneill and gkatsev May 9, 2023 03:45
src/videojs-http-streaming.js Outdated Show resolved Hide resolved
src/xhr.js Outdated
}

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

@adrums86
Copy link
Contributor Author

adrums86 commented May 9, 2023

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

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

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

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.

README.md Outdated

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.

README.md Outdated
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.

@adrums86
Copy link
Contributor Author

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

@adrums86 adrums86 merged commit 19539ea into main May 15, 2023
@adrums86 adrums86 deleted the fix-onRequest branch May 15, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants