twolfson / google-music.js

Browser-side JS library for controlling Google Music
The Unlicense
47 stars 6 forks source link

Add a callback to the constructor of GoogleMusic #16

Closed MarshallOfSound closed 8 years ago

MarshallOfSound commented 8 years ago

While attempting to come up with a more fluid testing process and while integrating into GPMDP I have noticed that sometimes we end up calling the new window.GoogleMusic(window) bit before Google Play Music has finished initializing and this throws some errors :cry:

I would like to suggest the the constructor takes a second parameter (a callback) and then waits for GPM to be ready before hooking into everything and firing the callback.

I'm happy to PR this if you think it is appropriate

twolfson commented 8 years ago

While the constructor isn't the best place for that (it's not very JS friendly), I understand your concern. In google-music-electron, we have a setTimeout loop that waits for all the selectors to load properly:

https://github.com/twolfson/google-music-electron/blob/2.5.0/lib/browser.js#L65-L108

However, I'm not sure it fits into the "one thing well" mentality of google-music.js since there's nothing specific except for new GoogleMusic(window). Instead, I would suggest using a retry library -- here's a bunch that fit the mold:

If you are curious why I wrote those lines instead of using a library, it was prob because it was faster than searching and was simple to test.

MarshallOfSound commented 8 years ago

Just because this was brought up again in #44

Trying to think of alternative strategies, what if we using a retry library internally and emitted a ready event when we successfully loaded.

Just an idea 😆

twolfson commented 8 years ago

If we can find one that's "small enough" (e.g. 10 lines of code-ish), then I would be okay adding a GMusic.ready(function (err, gmusic) { /* ... ready to go ... */ })

MarshallOfSound commented 8 years ago

So you desired syntax would be

GMusic.ready(function (err, gmusic) {
    // gmusic is an instance of GMusic
    gmusic.playback.playPause();
});

or

GMusic.ready(function (err) {
    var gmusic = new window.GMusic(window);
    gmusic.playback.playPause();
});
twolfson commented 8 years ago

The former, we don't want to assign window.gmusic It's the end user's variable to decide where to store On Jun 19, 2016 18:13, "Samuel Attard" notifications@github.com wrote:

So you desired syntax would be

GMusic.ready(function (err, gmusic) { // gmusic is an instance of GMusic gmusic.playback.playPause(); });

or

GMusic.ready(function (err) { var gmusic = new window.GMusic(window); gmusic.playback.playPause(); });

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gmusic-utils/gmusic.js/issues/16#issuecomment-227005628, or mute the thread https://github.com/notifications/unsubscribe/AA3FWHNVUpQBHd32mDFaqN7fZVHMLc0Jks5qNWqrgaJpZM4G0SwJ .

twolfson commented 8 years ago

Closing this longstanding issue as a wontfix. If someone does find that 10 LoC repo, then I would still consider adding it in.