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 support no longer seems to work? #40

Closed
whomwah opened this issue Jan 18, 2021 · 7 comments
Closed

TypeScript support no longer seems to work? #40

whomwah opened this issue Jan 18, 2021 · 7 comments

Comments

@whomwah
Copy link
Contributor

whomwah commented Jan 18, 2021

@jodal It looks like this commit seems to have broken things? unless it's just for me? This is change where the namespaces were switched from PascalCase to lowercase:

Lowercase namespace names

These are the lines in question:

Screenshot 2021-01-18 at 13 24 20

@jodal
Copy link
Member

jodal commented Jan 18, 2021

I have to admit that I've not familiar with namespaces in TypeScript, and see now that all the documentation on it use CamelCase naming.

Does your code work if you lowercase Mopidy too? mopidy is the name of the top-level TypeScript namespace. Mopidy is the name of the class.

@whomwah
Copy link
Contributor Author

whomwah commented Jan 18, 2021

Yeah, changing my code to mopidy doesn't fix things.

# Not working
import Mopidy from 'mopidy'
Mopidy.models.TlTrack 

import mopidy from 'mopidy'
mopidy.models.TlTrack

# Reverting commit Working
import Mopidy from 'mopidy'
Mopidy.models.TlTrack

Reverting the commit above makes it work again. I assume (as I am also no TypesFile expert) that by calling the namespace Mopidy (PascalCase) means that it is also exported and therefore makes .models for instance available a bit like class methods Klass.method.

Now I'm pretty sure that's not technically true :) but that's how I think of it.

@svedue
Copy link

svedue commented Jan 25, 2021

The mopidy namespace is not exported.
I fixed it locally by editing line 84
declare namespace mopidy {
to
export declare namespace mopidy {.
Then you can use
import { mopidy } from "mopidy";
and reference model with fqn:
trackDurationString(track: mopidy.models.Track)

@whomwah
Copy link
Contributor Author

whomwah commented Jan 26, 2021

One question on the above though. This seems a bit limiting and not quite right. I used to be able to do this before the change:

import Mopidy from 'mopidy'

const mopidy = new Mopidy({
  webSocketUrl: `ws://${mopidyUrl}:${mopidyPort}/mopidy/ws/`
})
const track: Mopidy.models.TlTrack = mopidy.playback.getCurrentTrack()

Now I'd have to do something like:

import Mopidy, { mopidy } from 'mopidy'

const mopidyInstance = new Mopidy({
  webSocketUrl: `ws://${mopidyUrl}:${mopidyPort}/mopidy/ws/`
})
const track: mopidy.models.TlTrack = mopidyInstance.playback.getCurrentTrack()

Have I missed something.

@jodal
Copy link
Member

jodal commented Jan 26, 2021

I still don't feel entirely up to speed on external TypeScript declarations, but given that:

  1. All the examples in the TypeScript documentation seem to use namespaces with an uppercase initial letter.
  2. Only having to import one thing is nice.
  3. Not having to import another thing that collides with the usual name of the Mopidy instance is nice.

I'm leaning towards reverting at least the top-most namespace to be named Mopidy instead of mopidy. Does that sound like an OK solution?

On an aside, I'd love to have a TypeScript linter and to have an example using TypeScript in the repo so that I have an easy way of testing these changes before releasing them. PRs welcome!

@whomwah
Copy link
Contributor Author

whomwah commented Jan 26, 2021

I know DefinitelyTyped require tests for the definition file, but TBH I'm not super sure how that works. I'll look into it though, as something like that sounds like what you're looking for.

@jodal
Copy link
Member

jodal commented May 6, 2021

Fixed by #45.

@jodal jodal closed this as completed May 6, 2021
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

No branches or pull requests

3 participants