lufinkey / react-native-spotify

A react native module for the Spotify SDK. [Deprecated]
377 stars 76 forks source link

Implement new Remote Spotify SDK #78

Closed cjam closed 5 years ago

cjam commented 6 years ago

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:

lufinkey commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

cjam commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

cjam commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Gotcha, sounds good.

cjam commented 6 years ago

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 commented 6 years ago

Currently there isn't a way

cjam commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

cjam commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

sweetcoco commented 5 years ago

Hey guys, just found this thread and excited about the direction of it. Any updates?

lufinkey commented 5 years ago

I've looked into this a bit, and I think it could potentially work if I could write a build script that renames the symbol in the outputted binary before linking

cjam commented 5 years ago

Sorry I have been busy with school and work so I havenā€™t had a chance to do any more work on this. I think we can probably offload the authentication stuff onto the new streaming sdk as it authenticates either with the app on your phone or a web view. I just havenā€™t had a chance to take a look at the API thatā€™s been provided in this package for the auth flow.

As far as updates. Ive got the iOS remote sdk is more or less working. Iā€™ve ran into a bug with maintaining the connection with the app. But itā€™s pretty close I think. Just need to do the android implementation

lufinkey commented 5 years ago

Does it do automatic token refresh? That's been my biggest headache

cjam commented 5 years ago

I believe there is a way it can be configured to do that yep

cjam commented 5 years ago

Alright, so just to update this issue with some details as to where things are at. I've got an IOS implementation for this and have been developing against it for a while now. For the most part it works well. There's one bug with the lib becoming disconnected and can't reconnect back to the Spotify App.

As for details pertaining back to this package. We unfortunately can't bring it in as is, because it has an auth class in it that clashes with the namespace in the original AuthSDK. Spotify however, removed dependence on these auth classes within their Streaming and Metadata SDK's by providing a parameter for an access token. I've already pulled out the Auth portion from the Remote portion in my implementation so that they are in different modules. The new SDK seems to support the either authorizing via the SpotifyApp (which works really well) or documentation says that they will fallback to a web based auth, but I've been unable to see that happen at this point.

If all of the streaming & metadata portions of this library take auth tokens, then we should be able to replace the Auth portion with the functionality offered in the new Remote SDK. I'm currently providing a very simple interface that takes config and returns back a token. Then the remote module will take a token and initialize its connection to the App.

Just wanted to add an update in here as it's been a while since I've reported back on the progress.

lufinkey commented 5 years ago

@cjam When you have the opportunity, can we take time to talk about the changes you're making before you make the PR? (outside of this GH issue would be preferable) Just because it's such a large change and I've made several commits here since your work started, so I want to make sure we're reconciling the two versions. I love the work you're doing so far btw.

cjam commented 5 years ago

Yea definitely. I still need to go and clean everything back up as I originally started by trying to replace the sdk until I realized that they serve very different purposes. Anyways, yes a chat would be good. We can set something up via email colter.mcquay@gmail.com

Colter


From: Luis Finke notifications@github.com Sent: Friday, February 1, 2019 6:33 AM To: lufinkey/react-native-spotify Cc: Colter McQuay; Mention Subject: Re: [lufinkey/react-native-spotify] Implement new Remote Spotify SDK (#78)

@cjamhttps://github.com/cjam When you have the opportunity, can we take time to talk about the changes you're making before you make the PR? (outside of this GH issue would be preferable) Just because it's such a large change and I've made several commits here since your work started, so I want to make sure we're reconciling the two versions. I love the work you're doing so far btw.

ā€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/lufinkey/react-native-spotify/issues/78#issuecomment-459741728, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA9DYKDXVG3pVMRA87dmwMurTBU9YReDks5vJFBPgaJpZM4XE1ux.

gabrielgrover commented 5 years ago

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

cjam commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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.

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 commented 5 years ago

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 commented 5 years ago

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 :+1:

lufinkey commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

Pros:

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