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

Improve typings #2322

Merged
merged 6 commits into from Dec 11, 2017
Merged

Conversation

aj-r
Copy link
Contributor

@aj-r aj-r commented Dec 6, 2017

Fixes #2317.

  • Never use the Object type. We should use object instead, or better yet, define a proper interface for those objects.
  • Never use the Function type. Instead, define a more specific signature for that function.
  • Changed several instances of type: string to more specific types (e.g. type: 'video' | 'audio'). This way typescript can catch typos, e.g. mediaPlayer.getTracksFor('vidoe') will cause a compile error.
  • Added special overloads for on(). This way listeners automatically get the right event type. e.g.
    mediaPlayer.on(MediaPlayer.events.TEXT_TRACKS_ADDED, e => {
        // 'e' implicitly has a type of TextTracksAddedEvent.
        // So the properties e.type, e.enabled, e.index, and e.tracks are available,
        // but any other property will raise a typescript compile error.
        this.tracks = e.tracks;
    });
  • I didn't add many JSDoc comments for now to make the diff easier to read. I'll probably add them in a separate PR.

@@ -2,13 +2,14 @@ export = dashjs;
export as namespace dashjs;

declare namespace dashjs {
class Debug {
interface Debug {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically Debug is an interface, not a class. This is because Debug objects are created with object literals ({}), not with a constructor (new Debug()).

index.d.ts Outdated
@@ -20,16 +21,16 @@ declare namespace dashjs {
}

export interface MediaInfo {
id: number | null;
id: string | null;
Copy link
Contributor Author

@aj-r aj-r Dec 6, 2017

Choose a reason for hiding this comment

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

I'm pretty sure id should be a string, not a number. Can someone confirm?

@epiclabsDASH epiclabsDASH added this to the v2.6.5 milestone Dec 6, 2017
@epiclabsDASH
Copy link
Contributor

Good work @aj-r. Regarding typings, there is still space for improvement (some properties still declared as any, full lack of types for metrics, etc) but this is going in the good direction.

Thanks for the change.

@epiclabsDASH epiclabsDASH merged commit 5c92243 into Dash-Industry-Forum:development Dec 11, 2017
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.

None yet

2 participants