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 isSupported check in browsers missing SourceBuffer global #2490

Merged
merged 4 commits into from Jan 6, 2020

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Jan 3, 2020

This PR will...

  • Fix isSupported check in browsers missing SourceBuffer global.
  • Add a eslint rule to prevent SourceBuffer global usage.
  • Update TypeScript and run npm audit fix

Why is this Pull Request needed?

JavaScript... no exceptions.

Are there any points in the code the reviewer needs to double check?

No

Resolves issues:

resolves #2430
resolves #2476

Checklist

  • changes have been done against master branch, and PR does not conflict
  • N/A new unit / functional tests have been added (whenever applicable)
  • N/A API or design changes are documented in API.md

@robwalch robwalch added this to the 0.13.1 milestone Jan 3, 2020
tjenkinson
tjenkinson previously approved these changes Jan 3, 2020
Copy link
Member

@tjenkinson tjenkinson left a comment

Choose a reason for hiding this comment

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

👍

@@ -5,7 +5,7 @@ export function isSupported (): boolean {
if (!mediaSource) {
return false;
}
const sourceBuffer = SourceBuffer || (window as any).WebKitSourceBuffer;
const sourceBuffer = (self as any).SourceBuffer || (self as any).WebKitSourceBuffer as SourceBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const sourceBuffer = (self as any).SourceBuffer || (self as any).WebKitSourceBuffer as SourceBuffer;
const sourceBuffer = self.SourceBuffer || (self as any).WebKitSourceBuffer as SourceBuffer;

self.SourceBuffer already has the correct type

Copy link
Collaborator Author

@robwalch robwalch Jan 5, 2020

Choose a reason for hiding this comment

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

Requires a TypeScript update (done 2c4f7e6). I was getting this error and went for the easy fix instead of updating deps.

$ npm run type-check

> tsc --noEmit

src/is-supported.ts:8:29 - error TS2339: Property 'SourceBuffer' does not exist on type 'Window'.

8   const sourceBuffer = self.SourceBuffer || (self as any).WebKitSourceBuffer as SourceBuffer;

Copy link
Member

Choose a reason for hiding this comment

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

Oh maybe. I just checked locally in vscode and it had the types. If it doesn’t cause any issues I think keeping typescript up to date makes sense

@@ -76,6 +76,13 @@ module.exports = {
'never'
],

'no-restricted-globals': [2,
{
'name': 'SourceBuffer',
Copy link
Member

Choose a reason for hiding this comment

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

👍

itsjamie
itsjamie previously approved these changes Jan 4, 2020
@robwalch robwalch dismissed stale reviews from itsjamie and tjenkinson via 2c4f7e6 January 5, 2020 18:26
@robwalch robwalch merged commit 3b16e49 into master Jan 6, 2020
@robwalch robwalch deleted the bugfix/source-buffer-exception branch January 6, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants