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

Enh/update videojs #1318

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Enh/update videojs #1318

wants to merge 8 commits into from

Conversation

SebiWrn
Copy link
Collaborator

@SebiWrn SebiWrn commented Jan 23, 2024

Motivation and Context

Video.js has a new major version and we should update it for security and functionality purposes.

Description

Updated Video.js and fixed some new errors that occured due to a new system on the video.js side. Some workarounds because Video.js isn't really compatible to TypeScript, but only to JavaScript.

Also fixes "Prototype Pollution in JSON5 via Parse Method"

@SebiWrn SebiWrn self-assigned this Jan 23, 2024
@SebiWrn SebiWrn marked this pull request as ready for review January 23, 2024 19:16
@SebiWrn
Copy link
Collaborator Author

SebiWrn commented Jan 23, 2024

Testserver is online at https://1318.test.live.mm.rbg.tum.de/

@SebiWrn SebiWrn requested a review from barisgul15 January 24, 2024 15:08
YiranDuan721
YiranDuan721 previously approved these changes Jan 24, 2024
Copy link
Contributor

@YiranDuan721 YiranDuan721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works great! Thank you, it feels so good not to see warnings about vulnerabilities all the time.
But I would suggest to wait for another approval review, since this looks not like a minor change and may have side effects that I'm not awared of...

@YiranDuan721 YiranDuan721 dismissed their stale review January 24, 2024 21:29

Just found out that there might be some problem with the split view

@YiranDuan721 YiranDuan721 self-requested a review January 24, 2024 21:43
Copy link
Contributor

@YiranDuan721 YiranDuan721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upgrade doesn't seem quite perfect yet. I get these errors during compiling:

Compile errors modules by path ./ts/*.ts 53.3 KiB modules with errors 34.4 KiB [errors] ./ts/TUMLiveVjs.ts 20.8 KiB [built] [code generated] [2 errors] + 3 modules + 5 modules modules by path ./ts/entry/*.ts 682 bytes ./ts/entry/video.ts 388 bytes [built] [code generated] ./ts/entry/interactions.ts 294 bytes [built] [code generated] modules by path ./ts/components/*.ts 12.4 KiB ./ts/components/video-sections.ts 1.91 KiB [built] [code generated] ./ts/components/chat.ts 10.5 KiB [built] [code generated] [1 error] ./ts/video/watchers.ts 831 bytes [built] [code generated] [1 error]

ERROR in /home/dyr/gocast/web/ts/components/chat.ts
./ts/components/chat.ts 10:9-22
[tsl] ERROR in /home/dyr/gocast/web/ts/components/chat.ts(10,10)
TS2614: Module '"video.js"' has no exported member 'VideoJsPlayer'. Did you mean to use 'import VideoJsPlayer from "video.js"' instead
?
@ ./ts/entry/interactions.ts 2:0-35 2:0-35

ERROR in /home/dyr/gocast/web/ts/TUMLiveVjs.ts
./ts/TUMLiveVjs.ts 3:18-31
[tsl] ERROR in /home/dyr/gocast/web/ts/TUMLiveVjs.ts(3,19)
TS2614: Module '"video.js"' has no exported member 'VideoJsPlayer'. Did you mean to use 'import VideoJsPlayer from "video.js"' instead
?
@ ./ts/entry/video.ts 3:0-30 3:0-30

ERROR in /home/dyr/gocast/web/ts/TUMLiveVjs.ts
./ts/TUMLiveVjs.ts 237:41-47
[tsl] ERROR in /home/dyr/gocast/web/ts/TUMLiveVjs.ts(237,42)
TS2339: Property 'extend' does not exist on type 'typeof videojs'.
@ ./ts/entry/video.ts 3:0-30 3:0-30

ERROR in /home/dyr/gocast/web/ts/seekbar-highlights.ts
./ts/seekbar-highlights.ts 3:9-22
[tsl] ERROR in /home/dyr/gocast/web/ts/seekbar-highlights.ts(3,10)
TS2614: Module '"video.js"' has no exported member 'VideoJsPlayer'. Did you mean to use 'import VideoJsPlayer from "video.js"' instead
?
@ ./ts/watch.ts 134:0-69 134:0-69 134:0-69
@ ./ts/entry/video.ts 4:0-25 4:0-25

ERROR in /home/dyr/gocast/web/ts/hotkeys.ts
./ts/hotkeys.ts 24:58-71
[tsl] ERROR in /home/dyr/gocast/web/ts/hotkeys.ts(24,59)
TS2694: Namespace 'videojs' has no exported member 'KeyboardEvent'.
@ ./ts/TUMLiveVjs.ts 6:0-42 130:21-34
@ ./ts/entry/video.ts 3:0-30 3:0-30

ERROR in /home/dyr/gocast/web/ts/hotkeys.ts
./ts/hotkeys.ts 34:30-41
[tsl] ERROR in /home/dyr/gocast/web/ts/hotkeys.ts(34,31)
ERROR in /home/dyr/gocast/web/ts/video/watchers.ts
./ts/video/watchers.ts 1:9-22
[tsl] ERROR in /home/dyr/gocast/web/ts/video/watchers.ts(1,10)
TS2614: Module '"video.js"' has no exported member 'VideoJsPlayer'. Did you mean to use 'import VideoJsPlayer from "video.js"' instead
?
@ ./ts/components/video-sections.ts 3:0-56 14:12-31
@ ./ts/entry/video.ts 8:0-45 8:0-45

9 errors have detailed information that is not shown.
Use 'stats.errorDetails: true' resp. '--stats-error-details' to show it.

webpack 5.89.0 compiled with 9 errors in 1989 ms


The player stucks when I try to interact with the progress bar. This can be reproduced in the testserver link.

@SebiWrn
Copy link
Collaborator Author

SebiWrn commented Jan 25, 2024

I think the build error is only locally on your system because some files aren't updated, because VideoJsPlayer isn't used in any file anymore.

@YiranDuan721
Copy link
Contributor

YiranDuan721 commented Jan 25, 2024

I think the build error is only locally on your system because some files aren't updated, because VideoJsPlayer isn't used in any file anymore.

That's possible, I will do more checks locally. But the problem

The player stucks when I try to interact with the progress bar.

does occur on https://1318.test.live.mm.rbg.tum.de/. Maybe the build environment for test servers also has the same problem as my environment?

@SebiWrn
Copy link
Collaborator Author

SebiWrn commented Jan 29, 2024

Now I have the same problem locally, never had it before, I'll have a look into it

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.

2 participants