-
Notifications
You must be signed in to change notification settings - Fork 8
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 to generator 8 + node 14 #2
base: master
Are you sure you want to change the base?
Conversation
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.
One comment about getting a url extension. We also need to turn on github actions here so that the tests can run. I don't have permissions to do it though.
|
||
setUpSkips(player); | ||
const getType = (url) => { | ||
const ext = url.split(/[#?]/)[0].split('.').pop().trim().toLowerCase(); |
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.
perhaps we should directly import https://github.com/videojs/video.js/blob/main/src/js/utils/url.js#L123
from video.js here? It will duplicate the the function when included with video.js globally, but I think that It would be better to use a well tested central piece of code. Ideally we would just export all of the utils that video.js has globally. I will add an issue to the major version of video.js ticket that we have (videojs/video.js#7068)
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 don't think we should be exporting everything necessarily. Each thing that's exported means that we can't easily change the method without it potentially being a breaking change.
However, in this case, we already expose the isCrossOrigin and parseUrl from url.js, and getFileExtension
hasn't changed much, so, we're likely safe in exporting it. Can also be exported in a minor.
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 guess this should probably be a major because it changes package structure.
Updates from generator 8
Minor changes to plugin for current lint standards
Replaces
path
- I was going to do refactoring in a separate PR, but it's easier to take it out now. With this, min.js is 2433 rather than 9339 bytes.