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

feat: Allow xhr override globally #1059

Merged
merged 3 commits into from
Feb 9, 2021
Merged

feat: Allow xhr override globally #1059

merged 3 commits into from
Feb 9, 2021

Conversation

alex-barstow
Copy link
Contributor

Description

It is not currently possible to override or add any custom XHR logic at initialization, since the xhr() method is not exposed on the tech until the source is set (see) and uses a reference to the original videojs.xhr().

Although it is unadvisable in most cases (as mentioned in the docs), it is still useful to have the ability to do it, and the existing beforeRequest() hook is insufficient in many cases.

Specific Changes proposed

  • Substitute a custom videojs.Vhs.xhr function for videojsXHR, if it is provided
  • This PR does not add any documentation, as I wasn't sure if it is something we want to document

misteroneill
misteroneill previously approved these changes Feb 3, 2021
@alex-barstow
Copy link
Contributor Author

I realized that Function.name is unsupported in IE11, and may not be a reliable source of truth if the code is minified. Might be better to set a flag when overriding: videojs.Vhs.xhr.override = true. which @gkatsev had mentioned. Thoughts?

@gkatsev
Copy link
Member

gkatsev commented Feb 3, 2021

Oh, didn't realize that Function.name wasn't available on IE11 because why would it be available there...
I think it's better to add a flag to the XhrFunction ourselves and if it's present we know it hasn't been overridden.

@gkatsev
Copy link
Member

gkatsev commented Feb 3, 2021

Also, we can do the flag just for IE11 so that we can easily delete it once we drop IE11

src/xhr.js Show resolved Hide resolved
Co-authored-by: Gary Katsevman <me@gkatsev.com>
@gkatsev
Copy link
Member

gkatsev commented Feb 9, 2021

One thing I tried to do while testing is override it but then call the old xhr method, which caused a whole host of issues. Though, I don't think we need to fix it because this should only ever be used in extreme circumstances and comes with no warranty.
Only thing I can think of is whether we want to maybe log a "I sure hope you know what you're doing" the first time we encounter an overriden xhr method.

The code was as follows:

          const old = videojs.Vhs.xhr;

          videojs.Vhs.xhr = (...args) => {
            console.log('overriden!', args[0].uri);
            return old(...args);
          };

@gkatsev gkatsev merged commit 6279675 into main Feb 9, 2021
@gkatsev gkatsev deleted the allow-xhr-override branch February 9, 2021 18:57
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.

4 participants