-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: deprecate mergeOptions() and use merge() instead #138
Conversation
Maybe I should also update video.js version in package.json from "6 | 7" to "8". @gkatsev any thoughts? |
@gjanblaszczyk I was just thinking about this that this would be a breaking change but maybe not an issue. Yeah, if we want to keep it as a breaking change, we should update the package.json. Alternative is to use |
@gkatsev I removed v6 from package.json. Is that fine? or maybe It is still needed... |
Yeah, that would work, though, technically still a breaking change. |
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, let's just do a breaking change here. @gjanblaszczyk would you mind making this compatible with ^8
only and removing the mergeOptions
fallback?
Let's keep it simple!
@misteroneill Sure, I don't have a problem with that. I have just updated the PR. Does it look fine? |
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
===========================================
+ Coverage 77.77% 95.00% +17.22%
===========================================
Files 3 3
Lines 81 80 -1
Branches 8 7 -1
===========================================
+ Hits 63 76 +13
+ Misses 18 4 -14
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Description
The video.js mergeOptions() function is deprecated. The plugin should use videojs.obj.merge() function.
Specific Changes proposed
Requirements Checklist