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

expose type for spotify web api object #101

Merged
merged 1 commit into from
Aug 23, 2018
Merged

Conversation

amelialaundy
Copy link
Contributor

@amelialaundy amelialaundy commented Aug 8, 2018

related to #44 (comment)

I would like to be able to return the instantiated client object as opposed to creating a new one each time or keeping the reference to the object within a class as so:

import * as SpotifyWebApi from 'spotify-web-api-js'

export const InitiateSpotify = (token: string): SpotifyWebApi.SpotifyWebApiJs  => {
    const client = new SpotifyWebApi();
    client.setAccessToken(token);
    return client;
}

therefore:

import * as SpotifyWebApi from 'spotify-web-api-js'
.....
class App extends React.Component<any, IState> {
  private spotifyClient: SpotifyWebApi.SpotifyWebApiJs;
  
  public onTokenSubmit = (token: string) => {
    this.spotifyClient = InitiateSpotify(token);

  }
...

This allows me to access all of the methods on the client object from calling functions and assign a type to this.spotifyClient previously you could not access SpotifyWebApi.SpotifyWebApiJsas per the issue linked above

The type is not available in the bindings currently as only the methods are exported, I'd like to propose something like this, this code change seems to work as I would like it to.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.378% when pulling 0da9811 on amelialaundy:master into 9a36af0 on JMPerez:master.

@JMPerez
Copy link
Owner

JMPerez commented Aug 8, 2018

Thanks for your PR @amelialaundy! Since I’m not very familiar with TypeScript I would like someone else’s input to review this, though it sounds good to me.

@amelialaundy
Copy link
Contributor Author

Yeah me too, do you know of someone? otherwise I can find some one to review it

@tobi12345
Copy link

love the new feature
cant wait for the merge 👍

@amelialaundy
Copy link
Contributor Author

thanks @tobi12345
just bumping this @JMPerez do you need me to find someone to review it? Or do either of you @JMPerez / @tobi12345 know anyone who can?

@JMPerez
Copy link
Owner

JMPerez commented Aug 22, 2018

@amelialaundy Let's merge this! We can always amend it if's not ideal.

I just wonder what the changes in package.json are about. Are those intended?

@amelialaundy
Copy link
Contributor Author

@JMPerez its the package-lock.json file that gets updated everytime a user does something with npm that changes node_modules or package.json, i.e npm install, so is expected

@JMPerez
Copy link
Owner

JMPerez commented Aug 23, 2018

I don't think package-lock.json changes unless you install a new dependency, or force it to update by removing the file and then running npm install again, right?

@amelialaundy
Copy link
Contributor Author

Kind of, if any packages have a hat ^ at the beginning in package.json, when I run npm install and there is a higher version than the one stated it will install a newer version of that eg:
https://github.com/JMPerez/spotify-web-api-js/blob/master/package.json#L21
so Sinon version in package.json is at "^5.0.0" and when I would have run npm install, the newest sinon version would have been installed @ 6.1.51, thus changing the package-lock.json file to show that install state

@JMPerez
Copy link
Owner

JMPerez commented Aug 23, 2018

That's interesting. The package-lock.json file locks the version of the package to be installed, so even if there are package updates compatible with the dependencies defined in package.json it will install the ones defined in the lock file.

Actually, If I run npm install on a clean copy of the project it doesn't alter the lock file.

@amelialaundy
Copy link
Contributor Author

hmm, perhaps I was wrong, but I'm guessing changing the node_modules as per https://docs.npmjs.com/files/package-lock.json would apply in my case as I didn't have any node_modules on a fresh clone?

@amelialaundy
Copy link
Contributor Author

also looking at the diff the original package-lock had hats for the version and those have been bumped to the highest and then the hat removed hmmm

@JMPerez
Copy link
Owner

JMPerez commented Aug 23, 2018

I'm going to merge this. We can always continue working on the dependencies in another commit.

@JMPerez JMPerez merged commit ff4fea6 into JMPerez:master Aug 23, 2018
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.

4 participants