spotify / apps-tutorial

A Spotify App that contains working examples of the use of Spotify Apps API
https://developer.spotify.com/technologies/apps/
627 stars 146 forks source link

Promises in the API #24

Closed ForbesLindesay closed 10 years ago

ForbesLindesay commented 11 years ago

I couldn't find a good way to raise issues with the Spotify API, so this tutorial seemed like the best bet.

The Spotify API includes a Promise class (see here). I like promises so I'm pleased to see you embracing them in your API. The problem is that these promises don't behave in anything like the way that promises in JavaScript are expected to behave. Promises in JavaScript have been specified by Promises/A+

This specification is being adopted as a basis for DOM Promises. It also has wide support on es-discuss so it seems likely that this, or something similar, will be adopted once Promises become a native part of ecmascript (probably in ES7).

The specification also has a large number of existing implementations (https://github.com/promises-aplus/promises-spec/blob/master/implementations.md). All of these are fully inter-operable, so they provide useful tools that can help compose async operations across multiple different promises libraries.

If you do choose to go down this route, there is an extremely comprehensive test suite to help you to establish your compliance here

ForbesLindesay commented 11 years ago

/cc @adamyeats @trodrigues @domenic

adamyeats-zz commented 11 years ago

I absolutely agree with @ForbesLindesay here, I've been frustrated to find today that the API includes A+ spec-incompatible promises that do not seem to behave like any Promise I have dealt with before. The closest match I could think of were jQuery .deferred() promises, and @domenic has already covered (http://domenic.me/2012/10/14/youre-missing-the-point-of-promises/) why jQuery-like Promises are a bad idea.

@ForbesLindesay covered pretty comprehensively the reasons why you would want to adopt this. Here's how I would expect to traverse your API to create a playlist, add a track, and then play that playlist:

models.Playlist.create('My Hawt Playlist')
  .then(function (playlist) {
    return playlist.tracks.add('spotify:track:1cjZ4U6KYSR4hwqTnX5S7B');
  })
  .then(function (playlist) {
    return models.Player.playContext(playlist);
  })
  .then(onFulfilled, onRejected);
thelinmichael commented 10 years ago

Thanks a lot guys. It's been some time since this issue was opened - apologies for not getting back with a response. Issues in this project have unfortunately been neglected from time to time, and since this one doesn't directly relate to the tutorial, it's gone unanswered. I don't see that the promise implementation will change in the foreseeable future, so I'm going to close this issue. I'll forward a link to it to the people that were involved in the API's architectural decisions though, and let it be up to them to reopen this discussion. Thanks again.

ForbesLindesay commented 10 years ago

@thelinmichael can you point us in the direction of the correct place to file issues with the API?

There is even stronger justification for the suggested change now than there was 5 months ago since the Promises spec we were originally proposing has been accepted for ES6 (with a few extensions). What this means is that there are promises in the next version of JavaScript that are different from promises in Spotify's API. You are directly going against a language builtin. One of these has a carefully thought out API and design that has been openly discussed and debated, the other appears not to.