fix: remove global and use 'videojs-global-compat'#1595
fix: remove global and use 'videojs-global-compat'#1595ronalduQualabs wants to merge 5 commits intovideojs:mainfrom
Conversation
|
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
mister-ben
left a comment
There was a problem hiding this comment.
- The videos-global-compat probably ought to be in the videojs GitHub and NPM orgs.
- The lockfile version changes from v1 to v3. Not sure if that might add some unexpected changes or overheads to future maintenance.
- The old global package wasn't included in the builds because they're set up as externals in the rollup configuration. With the new package included, dist/videojs-http-streaming.js is 5.7% larger, min is 7%. That would need to change in the shared rollup configuration in videojs-generate-rollup-config:
- There are some replacements of "global" in test files that should be left. Probably not breaking, but confusing.
package.json
Outdated
| "mux.js": "7.1.0", | ||
| "video.js": "^7 || ^8" | ||
| "video.js": "^7 || ^8", | ||
| "videojs-global-compat": "^1.0.1" |
There was a problem hiding this comment.
The repo and package probably ought to be in the video.js GitHub and NPM orgs.
test/videojs-http-streaming.test.js
Outdated
| assert.equal(beforeLocalRequestCalled, 2, 'local beforeRequest was called twice ' + | ||
| 'for the media playlist and media'); | ||
| assert.equal(beforeGlobalRequestCalled, 1, 'global beforeRequest was called once ' + | ||
| assert.equal(beforeGlobalRequestCalled, 1, 'videojs-global-compat beforeRequest was called once ' + |
There was a problem hiding this comment.
| assert.equal(beforeGlobalRequestCalled, 1, 'videojs-global-compat beforeRequest was called once ' + | |
| assert.equal(beforeGlobalRequestCalled, 1, 'global beforeRequest was called once ' + |
test/videojs-http-streaming.test.js
Outdated
|
|
||
| assert.equal(onRequestHookCallCountGlobal, 0, 'no onRequest global hooks called'); | ||
| assert.equal(actualRequestUrlGlobal, undefined, 'global request url undefined'); | ||
| assert.equal(actualRequestUrlGlobal, undefined, 'videojs-global-compat request url undefined'); |
There was a problem hiding this comment.
| assert.equal(actualRequestUrlGlobal, undefined, 'videojs-global-compat request url undefined'); | |
| assert.equal(actualRequestUrlGlobal, undefined, 'globalrequest url undefined'); |
test/xhr.test.js
Outdated
|
|
||
| this.xhr(defaultOptions); | ||
| assert.equal(this.requests.shift().url, 'global', 'url changed with global override'); | ||
| assert.equal(this.requests.shift().url, 'videojs-global-compat', 'url changed with global override'); |
There was a problem hiding this comment.
| assert.equal(this.requests.shift().url, 'videojs-global-compat', 'url changed with global override'); | |
| assert.equal(this.requests.shift().url, 'global', 'url changed with global override'); |
test/xhr.test.js
Outdated
|
|
||
| videojs.Vhs.xhr.beforeRequest = () => { | ||
| return { uri: 'global-newOptions'}; | ||
| return { uri: 'videojs-global-compat-newOptions'}; |
There was a problem hiding this comment.
| return { uri: 'videojs-global-compat-newOptions'}; | |
| return { uri: 'global-newOptions'}; |
test/xhr.test.js
Outdated
|
|
||
| this.xhr(defaultOptions); | ||
| assert.equal(this.requests.shift().url, 'global-newOptions', 'url changed with global override'); | ||
| assert.equal(this.requests.shift().url, 'videojs-global-compat-newOptions', 'url changed with global override'); |
There was a problem hiding this comment.
| assert.equal(this.requests.shift().url, 'videojs-global-compat-newOptions', 'url changed with global override'); | |
| assert.equal(this.requests.shift().url, 'global-newOptions', 'url changed with global override'); |
test/xhr.test.js
Outdated
| xhrRequest = this.requests.shift(); | ||
|
|
||
| assert.equal(xhrRequest.url, 'global', 'url changed with global onRequest hooks'); | ||
| assert.equal(xhrRequest.url, 'videojs-global-compat', 'url changed with global onRequest hooks'); |
There was a problem hiding this comment.
| assert.equal(xhrRequest.url, 'videojs-global-compat', 'url changed with global onRequest hooks'); | |
| assert.equal(xhrRequest.url, 'global', 'url changed with global onRequest hooks'); |
test/xhr.test.js
Outdated
| this.xhr(defaultOptions); | ||
| xhrRequest = this.requests.shift(); | ||
| assert.equal(xhrRequest.url, 'global', 'url changed with player onRequest hooks'); | ||
| assert.equal(xhrRequest.url, 'videojs-global-compat', 'url changed with player onRequest hooks'); |
There was a problem hiding this comment.
| assert.equal(xhrRequest.url, 'videojs-global-compat', 'url changed with player onRequest hooks'); | |
| assert.equal(xhrRequest.url, 'global', 'url changed with player onRequest hooks'); |
test/xhr.test.js
Outdated
| assert.equal(response.headers.foo, 'bar', 'expected headers'); | ||
| assert.equal(response.statusCode, 200, 'expected statusCode'); | ||
| assert.equal(globalHookCallCount, 0, 'global response hooks not called yet'); | ||
| assert.equal(globalHookCallCount, 0, 'videojs-global-compat response hooks not called yet'); |
There was a problem hiding this comment.
| assert.equal(globalHookCallCount, 0, 'videojs-global-compat response hooks not called yet'); | |
| assert.equal(globalHookCallCount, 0, 'globalresponse hooks not called yet'); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1595 +/- ##
==========================================
+ Coverage 84.00% 84.02% +0.01%
==========================================
Files 44 44
Lines 11710 11710
Branches 2625 2625
==========================================
+ Hits 9837 9839 +2
+ Misses 1873 1871 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
remove global and use 'videojs-global-compat' to fix videojs/video.js#9103
If it's a feature or enhancement please be as detailed as possible.
If it's a bug fix, please link the issue that it fixes or describe the bug in as much detail.
Specific Changes proposed
Please list the specific changes involved in this pull request.
Requirements Checklist