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

Typescript types #33

Closed
wants to merge 2 commits into from
Closed

Typescript types #33

wants to merge 2 commits into from

Conversation

cristianpb
Copy link
Contributor

@cristianpb cristianpb commented Oct 7, 2020

Hi @jodal

Typescript is getting very popular, so I think this can be very useful for everyone and should fix #27

I took the types definition from #27 and add them to mopidy.js repository.

I also added a simple github CI workflow. It's nice to have all CI integrated in github, but if let you chose if you want this or travis.

Copy link
Contributor Author

@cristianpb cristianpb left a comment

Choose a reason for hiding this comment

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

I think there are some changes to do to mopidy types

Comment on lines +683 to +686
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
time_position,
}: {
tl_track: models.TlTrack;
time_position: number;

Comment on lines +699 to +702
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
time_position,
}: {
tl_track: models.TlTrack;
time_position: number;

Comment on lines +715 to +719
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
}) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
timePosition,
}: {
tl_track: models.TlTrack;
timePosition: number;
}) => void;
time_position,
}: {
tl_track: models.TlTrack;
time_position: number;
}) => void;

@kingosticks
Copy link
Member

Travis is essentially dead so we've been gradually moving our CI to CircleCI, we'd probably want to do the same here.

@LorenzKahl
Copy link

Hi there, I would really like to use these types in my project. Why isn't this being merged if I may ask?

@jodal
Copy link
Member

jodal commented Nov 7, 2020

@kingosticks wrote:

Travis is essentially dead so we've been gradually moving our CI to CircleCI, we'd probably want to do the same here.

I've converted this repo from Travis CI to GitHub Actions today. As the build didn't have anything in common with our Python projects and doesn't need the mopidy-ci Docker image to build, I found that GitHub Actions was probably the easiest thing to migrate to.

@LorenzKahl wrote:

Hi there, I would really like to use these types in my project. Why isn't this being merged if I may ask?

Sorry, I haven't been very on top of pull requests lately.

I've been having some doubts about adding TypeScript definitions, as one of the original design goals for Mopidy.js was to build an API for use from browsers that wouldn't require lots of maintenance, like what e.g. a REST API mapped to Mopidy's Core API would be. Thus, Mopidy.js uses introspection of the Mopidy Core API and is always up to date with the API on the Mopidy server it connects to.

When adding TypeScript definitions, we lock Mopidy.js (at least for TypeScript users) to a specific version of the Mopidy Core API. On the other hand, I've grown quite fond of TypeScript and would clearly have used it myself for building a web client, so I want Mopidy.js to support TypeScript. If the Mopidy Core API grows/changes, Mopidy.js users will have to help out updating the TypeScript definitions or use some as any to work around any mismatches between the actual API and the types.

I'll soon push a PR that integrates all the work from @altano, @whomwah, and @cristianpb.

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.

TypeScript Type Definitions: last mile?
4 participants