silvermine / videojs-quality-selector

MIT License
183 stars 54 forks source link

refactor: Use ES6 classes instead of videojs.extend #93

Closed absidue closed 1 year ago

absidue commented 1 year ago

video.js 8 removes the videojs.extend function and recommends using native ES6 classes. Recent versions of video.js already spit out deprecation warnings when that function is used. https://videojs.com/guides/videojs-7-to-8/#videojsextend

coveralls commented 1 year ago

Coverage Status

Coverage: 0.0%. Remained the same when pulling 68eb394cb745cb2b46fa55a960b1f4bbcddd0e1c on absidue:no-vjs-extend into a40e9a680382023bdde8b240e3093142b905343f on silvermine:master.

absidue commented 1 year ago

@yokuze looks like babelify was already introduced in https://github.com/silvermine/videojs-quality-selector/pull/86.

andreas-venturini commented 1 year ago

looks like babelify was already introduced in https://github.com/silvermine/videojs-quality-selector/pull/86.

@yokuze does this address your concerns re backwards compatibility?

gsimko commented 1 year ago

Can you please also update the icon to \f114 as done in pr #99? With videojs 8 it is showing the incorrect "download file" icon instead of the gears.

emileuge commented 1 year ago

Thanks so much because this truly fixed the issue I had of incompatibility between video.js last version and this plugin which rendered it unusable.

absidue commented 1 year ago

@gsimko Changing that icon to a video.js 8 specific one, will probably break it for video.js 7 and below.

gsimko commented 1 year ago

@absidue that's right but without adding video-js version-specific code it's not possible to maintain backward compatibility because it's a breaking change in video-js. In such cases, people often decide to cut a new semantic version and leave behind the burden of supporting the old version, but it's up to the owner of this package whether actively supporting old versions is desired.

cbarburescu commented 1 year ago

Are there any updates on this? Thank you

gsimko commented 1 year ago

Just to clarify, if the icon change needs further discussion, we should get this PR submitted without touching that part. Users can circumvent the icon problem, but not the breaking problem that this PR fixes.

andreas-venturini commented 1 year ago

My suggestion would be to get this PR merged and released, then open a follow-up PR that addresses the breaking change introduced in Video.js 8 with regard to the icon (this could then be a new major release that marks dropping Video.js 7 support as breaking change)

Matt is probably busy so maybe @pbredenberg or @onebytegone can decide whether this PR is good to go as is or needs additional work

This PR will resolve GH-94, GH-97, GH-98 and GH-101

josipRajkovicFlowout commented 1 year ago

Hello, thanks for all the work. Are there any updates to this? Any approximation as to when this might be merged?

yokuze commented 1 year ago

Thank you @absidue!

cokuna-pavelkosolapov commented 1 year ago

Hello, @yokuze can you please make an update by npm?

fguillen commented 1 year ago

In which version is this merged? CDNs are serving v1.1.2 :?

r152a commented 1 year ago

no changes in https://www.npmjs.com/package/@silvermine/videojs-quality-selector

alemosa commented 1 year ago

Hello, what's the status on this? On video.js 8.6.0 it's still showing the videojs.extend is not a function problem. Thanks

valentin-anamorphik commented 1 year ago

Hi,

Is the problem resolved?

r152a commented 1 year ago

Hi,

Is the problem resolved?

No. If I install via npm, the problem is not solved. That's why I can't upgrade to the new version of videojs.

andreas-venturini commented 11 months ago

As a workaround until a new release is available on npm: Yarn can fetch packages from a git repository. Include the following line in your project's package.json:

"videojs-quality-selector": "https://github.com/silvermine/videojs-quality-selector/archive/d16842b1acb16cb25f12404ce56d8577b1c61a8d.tar.gz",

Then overwrite the wrong font icon (download icon) to restore the cog icon again (as per https://github.com/silvermine/videojs-quality-selector/pull/93#issuecomment-1605032384):

.vjs-quality-selector {
  .vjs-icon-placeholder::before {
    content: "\f114";
  }
}
r152a commented 11 months ago

@yokuze When should we expect a new release in npm?