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

fix(types): Improve Typescript coverage #8148

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Feb 21, 2023

Updates to JSDOC, and to the Component constructor to improve the typescript definitions output. Hopefully addresses #8109 and #8141.

  • Some fixes to incorrect / incomplete JSDOC
  • Typescript doesn't understand JSDOC's {EventTarget~Event} syntax and produces broken output when it's encountered (type would end up as {EventTarget} but it also unexpectedly turns optional params into required params). These have had to be simplified to unscoped types.
  • Imports typedefs into a lot of files, player and tech especially.
  • Adds placeholder event methods (on etc) to Component. Typescript doesn't support Typescript's @mixes/@mixin and I've not found another way to get these included in the output other than having the methods present by default. These will be replaced or set to undefined. This approach doesn't seem great. I'm hoping there's a better alternative I'm missing here.

Unresolved

  • This does not resolve a couple of typescript errors with tracks (about how classes have been extended) and with videojs.VERSION.
  • This does not address adding types for videojs.Vhs
  • Typescript does not believe that a Component retrieved by getComponent() can be used as a contructor.

Reference
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #8148 (679f007) into main (a27ee05) will decrease coverage by 0.06%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main    #8148      +/-   ##
==========================================
- Coverage   82.02%   81.97%   -0.06%     
==========================================
  Files         110      110              
  Lines        7344     7351       +7     
  Branches     1773     1773              
==========================================
+ Hits         6024     6026       +2     
- Misses       1320     1325       +5     
Impacted Files Coverage Δ
src/js/big-play-button.js 27.58% <ø> (ø)
src/js/button.js 62.50% <ø> (ø)
src/js/clickable-component.js 90.00% <ø> (ø)
src/js/close-button.js 92.85% <ø> (ø)
...-bar/audio-track-controls/audio-track-menu-item.js 3.03% <ø> (ø)
src/js/control-bar/fullscreen-toggle.js 94.11% <ø> (ø)
src/js/control-bar/live-display.js 100.00% <ø> (ø)
src/js/control-bar/mute-toggle.js 94.59% <ø> (ø)
src/js/control-bar/picture-in-picture-toggle.js 79.31% <ø> (ø)
src/js/control-bar/play-toggle.js 84.84% <ø> (ø)
... and 70 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gkatsev
Copy link
Member

gkatsev commented Feb 28, 2023

I say :shipit: if it improves things.

@jdufresne
Copy link
Contributor

jdufresne commented Feb 28, 2023

I tested the PR locally. It resolves the original TypeScript issue for me that I reported in #8109. 👍

Thanks!

@gkatsev
Copy link
Member

gkatsev commented Feb 28, 2023

Thanks for confirming @jdufresne!

@mister-ben mister-ben merged commit 0022867 into videojs:main Mar 2, 2023
@LeeWhite187
Copy link

Is there a viable example of this library (video.js) in Angular 15, yet?
I'm having the same problems as this: 8141.
I walked through the issue links, and did not find a resolution.

Thanks in advance.

@gkatsev
Copy link
Member

gkatsev commented Mar 16, 2023

The workaround should be to set skipLibCheck in the tsconfig.

@LeeWhite187
Copy link

Yeah. I did that workaround...
But, setting that "skipLibCheck" flag also removes some of the benefits of working in Typescript.

Any idea when the Angular-friendly update will release?

@HarunDyn
Copy link

@LeeWhite187 +1

@gkatsev
Copy link
Member

gkatsev commented Mar 29, 2023

But, setting that "skipLibCheck" flag also removes some of the benefits of working in Typescript.

Yup, totally get it. It's not an ideal workaround.
One of the issues is that we thought that using types generation based on our pretty good jsdocs would be sufficient, but it seems it isn't. Especially with this change, most of the type errors go away, but there's still some stuff missing like exporting the types as expected from the main package.

We're considering the next steps for improved types support.

@LeeWhite187
Copy link

No worries. Thanks for the workaround in the meantime.

edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
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.

6 participants