emit success event after 200 response when playing track

[LinusU]

@TooTallNate you probably gonna have something to say about this aswell but I needed an event when I knew that I would receive data that should be decoded and sent to the speakers. I had problem with some assertion errors when I initialized lame and speaker and then got an error event without anything having being .pipe()ed to my stream.

[TooTallNate]

How about a generic "response" event right above the if check where you have the "success" event here? That way it would always get emitted... idk seems slightly more useful to me.

[LinusU]

Sound good to me, will update now.

Which do you prefer? emit('response', res.statusCode == 200) or emit('response', { success: (res.statusCode == 200) }).

[LinusU]

Or just expose the res object (or statusCode) but I felt that exposed to much of the implementation since we do have the BufferStream.

[TooTallNate]

I'm +1 for emitting the res object itself.

[LinusU]

Okay I pushed the changes now but just realized that I like the success-event more.

I thinks it's more useful since you don't have to do a if(res.statusCode == 200) check yourself and I think that this is going to be the must common use case and I don't really like that we expose the res object just because I see no use for it. And if we expose it now it might be more job to remove it (e.g. if people are already using it) if we somehow change the underlying implementation. I vote we don't merge this until anyone can give a valid usecase to when you want to use the res object.

[LinusU]

@TooTallNate @adammw any more thoughts?

[LinusU]

@TooTallNate @adammw ping!

[adammw]

i'm not really up-to-date with the rationale behind these patches so I'm not sure. that and I don't really have the time at the moment to go through it all. that being said, i'm hoping there's a more elegant solution.

[LinusU]

@adammw Sorry for disturbing you, do you mean a more elegant solution then emitting success? Or do you think that it isn't elegant to expose the res-object?

[adammw]

you didn't disturb me, don't take that comment as if you did anything wrong - i'm just letting you know that you don't have my full attention at the moment. I really liked the idea of play returning a stream directly rather than having to do a callback dance, but I can understand where your coming from and the problem you are facing. The other thing I liked about returning just a stream is that it abstracts away how we actually get the music data, e.g. we could be using a RTMP or P2P connection in the library rather than only HTTP, and you wouldn't know. I personally would blame upstream - ie. get them to fix their bug rather than working around it, but then again I know how much the "not our problem" infuriates users of a library. ok, sorry for the less than helpful rant, I really don't have a better alternative, i just wish one existed.

The only other thing I could think would be to make .play() accept a callback that gets a (err, stream) as the cb parameters. (Although this changes the API, this could still be backwards-compatible, in that play can still return a pass-through stream with an error event regardless of the callback. That and backwards-compatibility was really never an issue as the API is not stable).

[adammw]

@LinusU @TooTallNate how about track.play(cb) which has a callback with parameters function(err, stream, res)? And as I mentioned before, if there isn't an overhead or good reason not to, we can return the stream directly from track.play() so old code continues to work.

So new consumers can pass in a callback, do one thing (abort) if there was an error, or use the stream otherwise. The res object can be returned for completeness, however I would put a comment in for clients not to depend on it in future versions, leaving us open to implement non-HTTP music streaming if the need comes up.

[LinusU]

@adammw I like it!

However, I would prefer it not returning the res since I don't see any use-case for it and it seems better to just not expose it at all (since "leaving us open to implement non-HTTP music streaming if the need comes up.").

I'll update my code asap with a suggestion.

[LinusU]

Hmm, this will break #46 which I really liked...

I'll push a commit with a suggestion but don't pull yet.

[LinusU]

I actually think that I'm back to liking my original suggestion best, mostly because of the abort support. Feedback greatly appreciated!