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

Implement new Remote Spotify SDK #78

Closed
cjam opened this issue Oct 2, 2018 · 64 comments
Closed

Implement new Remote Spotify SDK #78

cjam opened this issue Oct 2, 2018 · 64 comments

Comments

@cjam
Copy link

cjam commented Oct 2, 2018

Spotify seems to be moving towards a more formal iOS SDK which can be found here: https://github.com/spotify/ios-sdk which is pointed at by the SDK that this package uses. Would be great to move over to the new API and take advantage of some of the new features.

I would be down to help with doing this but I thought I'd first see if:

  • A. maybe this work has already been started somewhere
  • B. Get any advice on local package development for this project
@lufinkey
Copy link
Owner

lufinkey commented Oct 2, 2018

I haven't done anything to update the project for the new SDK other than point it to the old, renamed repository https://github.com/spotify/ios-streaming-sdk for compatibility sake.

For local package development, I've found the easiest way to work on it is to make a test project, add this repo as a dependency, and then delete it in the node_modules and replace it with the cloned git repository. The only annoying part of this is that if you run npm install in the test project at all after you do this it'll delete the git repository in the node_modules, so be careful. It's a pretty annoying process but I haven't found a better way to develop and test react native modules.

@cjam
Copy link
Author

cjam commented Oct 2, 2018

gotcha, I wonder if this could be done using npm link / yarn link. I've used it in the past for local module development. Thanks for the suggestion.

Edit: For the record, the react-native packager doesn't respect symlinks so you indeed need copy the repo right into the node-modules 😬. I definitely overwrote some work by accident with a yarn add 🤦‍♂️

@jackcbrown89
Copy link

I would also be down to help with anything needed from the JS side of things.

@cjam
Copy link
Author

cjam commented Oct 7, 2018

So some updates on this one. Progress has been a bit slow as it's my first foray into Objective C, however, things are coming along. I'm starting to get the auth flow working with the new session manager. The new API is quite a deviation from the previous streaming SDK. It's all about controlling the installed Spotify app using a remote controlling protocol (i.e. requires Spotify to be running in the background). That being said, it does allow much of the heavy lifting around caching, downloading, offline playback, audio streams etc to be left in the hands of Spotify and thus removing a lot of related complexity from this react-native wrapper.

@lufinkey Have you checked out the new Spotify SDK? Since it's such a large deviation from the previous streaming-sdk I'm thinking it would make more sense setting up a separate package for this. Perhaps rn-spotify-remote-sdk?

@lufinkey
Copy link
Owner

lufinkey commented Oct 7, 2018

I'd say it's probably better to just create and merge into a separate branch on this repo, and try to keep the existing JavaScript API. I doubt they've removed any features, and the point of this library is to maintain uniformity between the iOS and Android SDKs

@lufinkey
Copy link
Owner

lufinkey commented Oct 7, 2018

That being said, if you're trying to build something new from the ground up, then by all means just make the new project and then create the new package when it's done. I just personally hesitate to make any functionality available on iOS that isn't available in Android because it defeats the purpose of using react native

@cjam
Copy link
Author

cjam commented Oct 7, 2018

I fully agree about keeping the package cross platform, they have an equivalent new Remote SDK for Android as well. These new SDK's have a different architecture from the previous streaming sdks.

Spotify IPC

So, it seems that the Spotify team is trying to expose the Spotify services through this interface to leverage all of the code that is in Spotify. Sending commands and events over the IPC. I'll take a look through the existing API that you've exposed in this repo and let you know what doesn't appear to exist anymore in the new SDKs.

@lufinkey
Copy link
Owner

lufinkey commented Oct 7, 2018

Sounds good, thanks! I wish I had the time to just go in and rewrite the code to fit with the new SDK but unfortunately school and other projects are taking up most of my time at the moment

@cjam
Copy link
Author

cjam commented Oct 7, 2018

So I haven't really gotten to querying for tracks / albums etc. But at first glance it seems that the new SDK doesn't support the metadata. I'm hoping to get into some of the querying parts of the new API today so I'll hopefully have some more info after I get into things.

Another option for this library, is that this new Spotify SDK could be added to this repo and could be exposed as different module within this package? Therefore this package could end up looking something like this:

import {Streaming, AppRemote} from 'rn-spotify-sdk';

@cjam
Copy link
Author

cjam commented Oct 7, 2018

It's all good. You did a great job on the initial package, so it gave me lots to follow in terms of patterns. So I thank you for that. The more I think about it, the more I feel that having both the streaming and remote sdk's in this package feels like it would be the most useful, so I'll continue under that premise for now. It can always get pulled apart later if it makes more sense.

@cjam
Copy link
Author

cjam commented Oct 7, 2018

@jackcbrown89

I would also be down to help with anything needed from the JS side of things.

How's your android/java? lol . A bit far from JS land, but just throwing it out there. It'll likely take me a while to get around to the Android side of things on this.

@lufinkey
Copy link
Owner

lufinkey commented Oct 7, 2018

The metadata shouldn't be hard to replace with javascript http requests, since that's pretty much all they are except native.

@cjam
Copy link
Author

cjam commented Oct 8, 2018

Quick question. Was there a reason for using react-native-events over using the event system provided by react-native https://facebook.github.io/react-native/docs/native-modules-ios#sending-events-to-javascript ? I'm happy to use either, just curious.

@lufinkey
Copy link
Owner

lufinkey commented Oct 8, 2018

The event system provided by react native required you to use different classes to subscribe to events on iOS and Android (or atleast it did at the time that I wrote it. It may be different now)

@lufinkey
Copy link
Owner

lufinkey commented Oct 8, 2018

Yeah just checked and they're still different. iOS uses NativeEventEmitter and Android uses DeviceEventEmitter. Not really clear on why they decided to make a distinction

@cjam
Copy link
Author

cjam commented Oct 8, 2018

Gotcha, sounds good.

@cjam
Copy link
Author

cjam commented Oct 10, 2018

Is there anyway from react-native-events to know when an event is subscribed to from native code? The new spotify api allows you to get PlayerState changes but you have to subscribe to them, and I'd like to subscribe/unsubscribe based on there being listeners on the javascript side. Thoughts?

@lufinkey
Copy link
Owner

Currently there isn't a way

@cjam
Copy link
Author

cjam commented Oct 12, 2018

Alright, I figured out a way around the event subscription piece. I left a comment in react-native-events describing how it works.

As for this work. The iOS side is growing more stable. I have all of the authorization flow and most of their Remote API mapped. I'm still missing a few things (renewing session, getting user capabilities / restrictions) and have a few things to cleanup to make it more robust. I have been using typescript to map out the shape of the API objects as well as provide some intellisense. I've been trying to keep only the properties on objects that exist in both the ios and java apis. But it's likely that when the java side gets implemented some structures will need to change shape a bit.

It also has the added benefit documentation auto-generation which is nice.

Either way, I'm not sure what the play is on this one. They have completely changed the architecture of the new SDK and the shape of the API's have changed significantly. My feeling is that it would be best to create a different repo/package for tracking the new Spotify-SDK. That way, if people need to have more granular control over the audio streams and spotify resources they can still use this library. But if they want to control the Spotify App they could use say rn-spotify-remote. Or they could theoretically use both if they so chose (not sure why they would do that).

It would be following the approach that Spotify took with their own SDK, i.e. creating a new repo to allow the other to remain for those who are still using it.

@cjam cjam changed the title Upgrade to latest SpotifyiOS SDK Upgrade to latest Spotify SDK Oct 15, 2018
@cjam
Copy link
Author

cjam commented Oct 16, 2018

@lufinkey I'm open to this being pulled into this repo as well. I just think that this package / SDK is still useful for applications that require more granular streaming / audio capabilities. The new remote one is useful for applications that just want / need to control spotify and pull cached resources straight from the app (i.e. offline scenarios)

@lufinkey
Copy link
Owner

lufinkey commented Oct 16, 2018

Yeah it might be better to eventually reorganize this package into something like { Auth, Player, Metadata, Remote }

@lufinkey lufinkey changed the title Upgrade to latest Spotify SDK Implement new Remote Spotify SDK Oct 16, 2018
@cjam
Copy link
Author

cjam commented Oct 18, 2018

How do you feel about Typescript? I'm a fan, especially for this type of work as you can codify the shapes of API's and JSON in a self documenting way. Some people seem to be very opposed to it. Up to this point I've been doing my implementation of the remote api with Typescript. Shouldn't change anything from the consumption standpoint and all of the tooling is based on npm packages / scripts. Just thought I'd ask.

@lufinkey
Copy link
Owner

I have mixed feelings honestly. I like the type checking and all the other things it offers, but at the same time, I'd like to keep the amount of dependencies low, especially with giant libraries like that. I'd say it's better for this if you just wrote it in JS and added some type checking ifs and such.

Honestly kinda wish deno was used more often instead of JS engines because type checking truly should be done by the language itself...

@cjam
Copy link
Author

cjam commented Oct 18, 2018

Never looked at deno before, that's pretty cool. Yea, I was only using typescript for development. It's not a requirement for using the library itself. It's just a dev dependency compiling it into a dist/ folder that get's committed just like you would with writing vanilla js. It should be transparent to the consumer, other than they would get typings automatically if they're using typescript and we could generate docs from the source trivially to take the burden out of updating the readme.

It can get converted back I'm sure at some point, but I'm just in the middle of trying to switch the package to something like import {remote,streaming} from 'rn-spotify-sdk' so I'll leave it for now and it can be switched if necessary.

@cjam
Copy link
Author

cjam commented Oct 18, 2018

ok, update on trying to include both the streaming and remote sdk's together. Unfortunately, the frameworks utilize some interfaces/classes that share the same names resulting in collisions when trying to compile. Darn. It seems that they re-used the object named SPTSession, however the interface isn't compatible with the previous.

That leaves us in a tough spot to combine the two.

@cjam
Copy link
Author

cjam commented Oct 24, 2018

So given that we can't compile the project with the previous Auth SDK and the new Spotify Remote SDK. We can either try to replace the auth portion of this library with the new Spotify Remote SDK. It is a bit different but handles authenticating against the app itself which is awesome and works offline. That being said, there are several items that would no longer exist in relation to getting back user information as the new Spotify SDK simply returns a session, but the session doesn't actually contain any user information. Just tokens, that can be used to retrieve any / all necessary information about who is represented by the session.

The ideal scenario would be what we discussed above around {remote, streaming}. I'm not sure what the streaming objects look like and if they require the session objects from the Auth framework. If they just require tokens, then it's possible we could separate into {remote, auth, streaming, web} where the auth portion is handled by the remote.

@lufinkey
Copy link
Owner

I am definitely against removing the behavior of the streaming SDK. The remote SDK looks a lot like Spotify trying to sandbox any 3rd party functionality to use only their app and be controlled entirely by them, which I'm not a fan of. Ideally we would be able to import both within the same package, but if there's no way to reconcile the different SPTSession objects, then it might just have to be an entirely separate react native package

@cjam
Copy link
Author

cjam commented Oct 24, 2018

yea agreed. I'm definitely not implying that the Streaming SDK should be removed. It seems that the namespace collisions happen between the Auth framework package specifically. They definitely broke the signature of the SPTSession, but luckily they had decoupled the metadata and streaming sdk's from the Auth by only requiring an access token. So I think it's possible the Authentication could be handled by the new SDK, but it will just take a bit more thought into how to split things up. I'll keep this issue posted with my findings. I've just been busy with other things at this point.

@cjam
Copy link
Author

cjam commented Feb 1, 2019 via email

@gabrielgrover
Copy link

Interested in this repo/thread. You guys have a fork/branch this new work is on? Would like to help out.

@cjam
Copy link
Author

cjam commented Feb 28, 2019

Hey guys, sorry I haven't been keeping this marching forward. I was able to put in a solid effort at the beginning on my fork but have been swamped with school and work and so I haven't been able to refactor my stuff to get it ready for reintegration.

I originally started this fork as a way for me to learn Objective-C, and I thought the remote-sdk would be a drop-in replacement to the previous SDK. This is not the case, however, I didn't learn this until I had shuffled around the structure of my fork.

Now that I've done the learning. I believe there is a way that these SDK's can coexist in one package. I, however, need to clean up and rebase my fork which is going to take me some time as I was hacking things apart pretty liberally during my learning phase.

That being said, the one area of conflict is the legacy Auth sdk and the new Remote SDK as they have namespace collisions. The new Remote SDK has a session manager that appears to take care of all of the Auth needs which I think might be able to replace the auth components (i.e AuthView, AuthViewController, etc). @lufinkey would you be able to take a look at https://github.com/spotify/ios-sdk/blob/23644b0fceb1d2b305a4319b19d8a09631cb9c48/DemoProjects/SPTLoginSampleApp/SPTLoginSampleApp/ViewController.m#L50

and let me know what you think? If we can agree that we'll be replacing auth with the mechanisms within the Remote SDK then I will spend some time cleaning up and rebasing, resolving conflicts etc.

@Nexuist
Copy link

Nexuist commented Feb 28, 2019

I've been anticipating the new Remote SDK for quite a long time, so I just wanted to say thank you for the hard work and I'm looking forward to being able to test it 👍

@cjam
Copy link
Author

cjam commented Feb 28, 2019

I've been anticipating the new Remote SDK for quite a long time, so I just wanted to say thank you for the hard work and I'm looking forward to being able to test it 👍

Yea sorry it's been so delayed, I've actually been testing it via my fork in a project I'm working on and it's working pretty well save a few bugs I haven't been able to figure out. It'll be good to get it in here and get some more eyes on it.

@lufinkey
Copy link
Owner

So given that I have to replace the iOS SPTAuth object anyway (it causes the player to pause every time the token refreshes, and has some other issues) I'm probably going to take a crack at getting this working. I need to do some refactoring at some point too before it happens but I just haven't had the time.

@cjam
Copy link
Author

cjam commented Feb 28, 2019

k sounds good. I'll try to find some time to get my stuff rebased and ready for reintegration then at least we can talk about it through a PR or something.

@lufinkey
Copy link
Owner

Sounds good. If you could, wait until I do the refactor to rebase, since I'm probably going to drastically change the structure pretty soon. I really need to separate out a lot of stuff.

@lufinkey
Copy link
Owner

lufinkey commented Mar 1, 2019

Also, @cjam you had mentioned using typescript. Recently I've started using Flow and I've been pretty impressed. The advantage is that it's built into react native, so no extra dependencies are needed. If we want to add types to the library at some point, I think it would be best to do it using that.

@cjam
Copy link
Author

cjam commented Mar 1, 2019

@lufinkey I haven't used flow before, as I've used Typescript from before flow existed. But I did recently see something on twitter about the Jest team moving Flow for Typescript: https://news.ycombinator.com/item?id=18918038.

Some points that I like about Typescript.

  • Very good support for 3rd party libaries
  • Very easy to define typings for libraries that don't have them (within your own project)

Also, just to clarify the use of Typescript in the context. If this project used Typescript, it wouldn't be required by projects that consume this package. In fact, consuming projects likely wouldn't notice a difference between the Javascript and Typescript versions except for the addition of the typings that come from Typescript. Typescript becomes a development dependency that is required for working on the project, as the build will compile ts files into Javascript files and output them to a /dist or build folder. I quite like it for wrapping API's because you can create interfaces that describe the shape of objects without actually outputting any javascript. As an example:

https://github.com/cjam/react-native-spotify/blob/spotify-remote-sdk/src/ContentItem.ts
Represents the following object from the Spotify SDK: https://spotify.github.io/ios-sdk/html/Protocols/SPTAppRemoteContentItem.html

I'm currently committing the output of the typescript build into my fork so that I can consume it via git and you can see the output here:

https://github.com/cjam/react-native-spotify/tree/spotify-remote-sdk/dist

Anyways, just some thoughts on the matter. I just wanted to make sure that you knew that Typescript wouldn't become a production dependency of this package.... because Typescript is huge haha.

@lufinkey
Copy link
Owner

lufinkey commented Mar 1, 2019

I'm aware it's a "dev dependency" but I'd still rather opt for something that's officially supported by react native.

I'm aware of how typescript works and the differences between the two, but given that I get a lot of people opening issues on this project struggling just to get the most basic aspects of react native working (literally just things like linking the project) I'd rather not add another point of failure that could cause me to have to respond to even more issues. Flow will just work without any setup or generated files in the project folder, and I'd like to stick to that.

@cjam
Copy link
Author

cjam commented Mar 1, 2019

Sorry didn't mean to insinuate that didn't know how typescript worked. Just wanted to make sure we were on the same page as I've never used flow and don't know what's involved or how it works. Have you used Flow on other projects? How did you find it scaled with your projects? The no config/setup option for react-native sounds pretty cool.

This decision was heavily considered at a previous employer I was at and ultimately the research pointed to Typescript. I can't tell you which is better as I only know Typescript. I've read a few articles here and there of people comparing them or discussing why they're switching from one to another. I'm sure you've done similar research on it and ultimately this is your repo so you get to make these types of decisions 👍

@lufinkey
Copy link
Owner

lufinkey commented Mar 1, 2019

No you're good. Sorry if that's how it came across. I wasn't intending to be rude. I've been using Flow in my main project (where this module is a dependency) and it scales really well. It's also really similar to typescript, so most things will translate pretty easily. It has its issues, like occasional bad error messages. It also builds and runs even if there are type errors, which makes it easier to deal with if you have issues.

@lufinkey
Copy link
Owner

lufinkey commented Mar 5, 2019

Some good news @cjam, I've managed to remove the SpotifyAuthentication.framework dependency on iOS, so we shouldn't have an issue with the SPTSession object conflicting anymore.

@cjam
Copy link
Author

cjam commented Mar 5, 2019

Nice. I've actually been thinking about this project and integration a lot lately. I took a step back from it and was trying to think of the problems that we're trying to solve with it, as lately I've been pretty wrapped up in finding a solution to getting these two SDK's to play nicely together. I know it's been discussed in the past but I think it needs some objective re-evaluation of the value behind combining these two SDK's into this package.

Cons:

  • These SDK's serve very different purposes and would likely not be used together
  • The increased application size in iOS: The Streaming SDK is approximately 25 MB (3.5 MB auth, 9.5 MB Streaming, 9.5 MB Metadata), the Remote SDK is approximately 15 MB. It's a lot of extra baggage the convenience of being able to use the library if you wanted.
  • Monolithic package concerned with all things Spotify. Mixes the concerns of the resulting package between Streaming and Remote vs two separate packages that specialize around their purpose.

Pros:

  • only have to install & link one package
  • centralized repo for all things react-native spotify

I'm trying to look at this objectively and am having trouble seeing the added value of combining the two in the long run.

@lufinkey
Copy link
Owner

lufinkey commented Mar 5, 2019

Yeah like I said before, I'm not opposed to having this as a separate package. Concerning the auth library, I was having several other issues with Spotify's auth library which is why I replaced it.

@cjam
Copy link
Author

cjam commented Mar 5, 2019

Gotcha, what issues were you having with it again?

@lufinkey
Copy link
Owner

lufinkey commented Mar 5, 2019

Basically, in order to implement the loginWithAccessToken method from #41 I would need to get the "canonical username" to pass to SPTAuth, and I didn't really want to make extra API calls to do that. I was also having an issue where the iOS player would stop whenever the session would refresh, but it turns out that this happens regardless, unfortunately.

@lufinkey
Copy link
Owner

lufinkey commented Mar 9, 2019

@cjam I can pretty likely eliminate the metadata framework from the list of dependencies, actually. It's really only being used for forming an HTTP request, which is something that can easily just be done within the library. It might still be worth adding the remote SDK

@lufinkey
Copy link
Owner

lufinkey commented Mar 9, 2019

Ideally we could add preprocessor directives that let you specifically include/disclude the remote SDK or the player SDK

@cjam
Copy link
Author

cjam commented Mar 9, 2019

Interesting. I think that to your point earlier in this thread around trying to minimize the difficulty in consuming the package from the end user standpoint. I can attest that it's a challenge getting it setup in Xcode as I've never found rn link to get it quite there. With the two libraries, there would be two different frameworks that would need to be linked depending on what you are using which I think would get confusing.

I think ultimately, the two SDK's serve different purposes that are pretty close to being mutually exclusive. With this package, you have the granularity necessary to create an app with a more immersive audio experience, vs the other SDK where you just want to tell the Spotify App to do stuff.

Yours is a great wrapper around the streaming SDK and I think trying to add another SDK in here would muddy the waters.

@lufinkey
Copy link
Owner

lufinkey commented Mar 9, 2019

Fair enough. I think as long as both SDKs allow you to manually login with an access token they should play well with each other anyways

@cjam
Copy link
Author

cjam commented Mar 9, 2019

Definitely agree.

@lufinkey
Copy link
Owner

lufinkey commented Mar 9, 2019

If you're going to make the remote SDK, would you be opposed to standardizing the Session object between our two libraries?

@cjam
Copy link
Author

cjam commented Mar 9, 2019

Not opposed, what did you have in mind? Do you mean from the javascript side (i.e. standardizing the shape of the contract)?

@lufinkey
Copy link
Owner

lufinkey commented Mar 9, 2019 via email

@cjam
Copy link
Author

cjam commented Mar 9, 2019

Yep that works. At the moment, I've got the Auth piece separate and just returning a token, which can then be used to startup a session. The session is managed internally by the Spotify Session Manager. But I've been trying to think about how a session could be persisted so that the app wouldn't need to do the Auth every time. The session object that's being stored internally is just the Spotify one:

https://spotify.github.io/ios-sdk/html/Classes/SPTSession.html

Perhaps this would be a good place to start?

@cjam
Copy link
Author

cjam commented Mar 14, 2019

Hey. So I pulled the remote stuff into its own repo and prefixed all of the names to avoid any conflicts with this library. I've published the first version of it here:

https://www.npmjs.com/package/react-native-spotify-remote

I also churned out some crude documentation for it (there's a link at the top of the Readme).

Some things I'm hoping to do soon are create an Example app for it. I think that'll make it easier to track down bugs and also provide a platform to start implementing the android side of things. Anyways, It's still got some issues but I thought I'd put it out there.

@lufinkey
Copy link
Owner

Awesome, i'll close this issue then

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

6 participants