brim-borium / spotify_sdk

Flutter Package to connect the spotify sdk
https://pub.dev/packages/spotify_sdk
Apache License 2.0
152 stars 83 forks source link

RFC: Minimise external dependencies #88

Open fotiDim opened 3 years ago

fotiDim commented 3 years ago

This is a request for comments. It seems to me that this library depends on too many external dependencies. In general there is little wrong with that but it can lead to some issues:

If this was an app I would be totally fine with any amount of dependencies but since this is a library my ideal amount of dependencies would be 0.

I was about to suggest to use the dart:io library instead of Dio but by reading further I realised that dart:io does not support Flutter web. The official documentation suggests the http package instead. Is there any benefit of using Dio? I would assume that the majority of apps are using the http package so migrating to the http package would mean one dependency less for most apps. Unfortunately I do not have any data to back up this claim but my gut feeling says that http is the most popular choice.

Are there any other packages we can get rid of?

brim-borium commented 3 years ago

I will have a look at this.

brim-borium commented 3 years ago

So what I found out so far:

I think we can replace dio with http, but I would go a step further and move the http calls to its own client for the web api, so this will be usable in the future if we want to extend the functionality of the sdk to provide web api connections-> using web api calls from android or ios. Regarding this @itsMatoosh are we currently using Web Playback SDK or the web api?

Further on minimising package use:

I think we can probably remove logger and write our own logging implementation. I don't see an alternative for json_annotation. Js, crypto and synchronized are being used in the web implementation @itsMatoosh can probably better check wether we can remove any of these. I think js probably is needed for web.

itsMatoosh commented 3 years ago

@fotiDim We could definitely remove dio and make use of http instead.

@brim-borium In the web implementation we are using both the Web Playback SDK and the Web API. The playback SDK only allows for creating a new player in the browser, but doesn't provide functions such as play(), pause() etc. So the player is created using the Web Playback SDK and controlled using the Web API.

Regarding the libraries used in the web implementation, they are all needed. JS makes it possible to call the Web Player SDK js API from dart. Crypto is used for generating a code challenge used for authentication. Synchronized is used to ensure that each call to getAuthenticationToken() waits until the last call to that method is finished, this has to do with how the Web SDK handles reauthentication.

fotiDim commented 3 years ago

@itsMatoosh regarding the Web Playback SDK according to this page it does support Control local playback (pause, resume, volume, etc). Are you sure the web api is needed for this?

Regarding Synchronized I understand what you are trying to achieve. Wouldn't something like this have the same effect?

itsMatoosh commented 3 years ago

@fotiDim Yes, these methods are implemented using the Web Playback SDK already, however methods such as play(Track) are not available on the Web Playback SDK and therefore I had to use the Web API for that.

The stream approach to make a queue of getAuthenticationToken requests seems interesting. I suppose we could implement it by using a StreamController that accepts some kind of a callback function and then having the getAuthenticationToken method would loop over all the token requests and resolve them one by one. The problem is, I think that might be less efficient than just using synchronized.

fotiDim commented 3 years ago

Loosely relevant topic, it seems we have some overlap with the spotify-dart package in the web implementation. For example authentication or some web API methods that we use like the play endpoint. Are there any parts that interest us that we could potentially adopt?

ViktorKirjanov commented 4 months ago

updates?