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

Chore: Update to generator 8 + node 14 #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mister-ben
Copy link

@mister-ben mister-ben commented Oct 5, 2021

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.

Copy link

@brandonocasey brandonocasey left a 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();

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)

Copy link
Member

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.

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.

I guess this should probably be a major because it changes package structure.

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.

3 participants