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

RFC: Minimise external dependencies #88

Open
fotiDim opened this issue Apr 5, 2021 · 7 comments
Open

RFC: Minimise external dependencies #88

fotiDim opened this issue Apr 5, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@fotiDim
Copy link
Collaborator

fotiDim commented Apr 5, 2021

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:

  • People rejecting this library because of the amount of external dependencies
  • People having version conflicts in their dependencies if they are including a different version of the same package
  • Increased overhead and perhaps redundancy in dependencies. Why should we include Dio if a project has decided to use a different networking library?
  • Maintaining this library is always going to be affected and perhaps slowed down by external dependencies. We had this case with the null safety migration.

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?

@fotiDim fotiDim added the enhancement New feature or request label Apr 5, 2021
@brim-borium
Copy link
Owner

I will have a look at this.

@brim-borium brim-borium self-assigned this Apr 10, 2021
@brim-borium
Copy link
Owner

brim-borium commented Apr 10, 2021

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

fotiDim commented Apr 16, 2021

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

fotiDim commented May 16, 2021

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
Copy link

updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants