-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
chore: Update preset env, drop IE11 and older browser support #7708
chore: Update preset env, drop IE11 and older browser support #7708
Conversation
src/js/extend.js
Outdated
|
||
// If the provided super class is a native ES6 class, | ||
// make the sub class one as well. | ||
if (/^class/.test(superClass.toString())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Video.js components are now un-transpiled ES6 classes, extend()
needs to be able to handle them since videojs.extend(videojs.getComponent('Component'), {...})
is the most common usecase for this method.
This string check is apparently the only way to detect ES6 classes. It isn't future proof and supposedly will not work on some older platforms, but that is likely not a huge concern since we are 1) dropping support for most older platforms and 2) deprecating this method and encouraging the use of native classes instead.
Codecov Report
@@ Coverage Diff @@
## next #7708 +/- ##
==========================================
+ Coverage 80.88% 80.90% +0.02%
==========================================
Files 116 116
Lines 7454 7459 +5
Branches 1806 1809 +3
==========================================
+ Hits 6029 6035 +6
+ Misses 1425 1424 -1
Continue to review full report at Codecov.
|
247a277
to
dc0331d
Compare
"not kaios 2", | ||
"not op_mini all", | ||
"not op_mob 64" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should we specify the minimum Safari version here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think last 3 major versions
covers Safari adequately.
@@ -1,19 +1,19 @@ | |||
WEBVTT | |||
|
|||
00:00.700 --> 00:04.110 | |||
00:00.700 --> line:0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file shouldn't have gotten checked in 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure what's going on here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was something I was playing around with back in the other PR and got copied over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove. I copied all changes over from your PR.
@@ -27,10 +28,20 @@ import _inherits from '@babel/runtime/helpers/inherits'; | |||
* The new class with subClassMethods that inherited superClass. | |||
*/ | |||
const extend = function(superClass, subClassMethods = {}) { | |||
log.warn('The extend() method is deprecated. Please use native ES6 classes instead.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add the deprecation to the jsdoc comments too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM, but wondering about that VTT file.
@@ -1,19 +1,19 @@ | |||
WEBVTT | |||
|
|||
00:00.700 --> 00:04.110 | |||
00:00.700 --> line:0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure what's going on here...
- remove IE-specific code, refer to videojs#7701 & videojs#7708 - remove remaining code to hide labels, refer to videojs#8101
Description
This duplicates all changes in #7482 and updates them to be consistent with the browserslist specified in videojs/babel-config, as well as fixes a few problems caused by the removal of ES5 transpiling.
NOTE: Merge with
BREAKING CHANGE
header