Fixes #15 - Event for when Spotify is open #39

s-h-a-d-o-w · 2018-05-26T22:05:01Z

This is a big one...
Due to the checks for whether Spotify is running, errors will not be emitted as they were before, hence the major version up. (I also used this opportunity to get rid of the deprecated event 'track-change')
After all, some people might have constructed apps based on those errors already - I know I did. Before it occurred to me that ideally, spotify-web-helper would emit events for that - and then I discovered #15...

I also introduced debug output using the debug package.
This is what the lifecycle looks like from the perspective of a user if they enable the debug messages (anything not prefixed with swh is the app that uses spotify-web-helper):

20:36:51.550:   swh Waiting for Spotify to start... +5s
20:36:56.662:   swh Waiting for Spotify to start... +5s
20:37:02.145:   swh Spotify was just started. +5s
20:37:02.145: Event 'open' emitted.
20:37:02.279: Process ID: 10992
20:37:06.068:   swh Starting to listen for events. +4s
20:37:06.337:   swh Processing status. +269ms
20:37:26.390:   swh Processing status. +20s
20:37:29.288:   swh Processing status. +3s
20:37:32.025:   swh Processing status. +3s
20:37:34.230:   swh Processing status. +2s
20:37:34.231: Event 'closing' emitted.
20:37:34.384:   swh Waiting for shutdown... +154ms
20:37:36.473:   swh Spotify quit, start waiting for connection again. +2s
20:37:36.474: Event 'close' emitted.
20:37:36.545:   swh Waiting for Spotify to start... +72ms
20:37:41.658:   swh Waiting for Spotify to start... +5s
20:37:46.777:   swh Waiting for Spotify to start... +5s
20:37:51.994:   swh Spotify was just started. +5s
20:37:51.994: Event 'open' emitted.
20:37:52.142: Process ID: 4640
20:37:56.465:   swh Starting to listen for events. +4s
20:38:02.178:   swh Processing status. +6s
20:38:02.425:   swh Processing status. +247ms
20:39:02.445:   swh Processing status. +1m

Another thing that's noteworthy is that for checking whether the process is running, I switched to using the package process-list - which uses native implementations and is faster than all other process list solutions I've benchmarked (including the "homegrown" exec wmic one).

Plus a number of smaller changes which I hope you will appreciate. Like introducing configurable interval settings for operations related to 'open' and 'close' events. But while I was it, I of course also included other intervals, like how long to wait with the next listen() after an error has occurred.

Plus: - Replaced manual wmic call on windows with process-list package (faster, one less thing to maintain here). - Debug output (channels 'swh:info' and 'swh:error') TODO: - 'Closing' might still receive a bit of tweaking - spotifyStatus with constants seems a bit ugly as it is right now

... status online=false is sent. Plus: Create an init() function instead of using SpotifyWebHelper constructor (which of course got rid of hooked up event handlers). Waiting for Spotify to run causes lower CPU usage and seems to cut down on errors sent by the service.

…erations Plus: Correct order in which compareStatus(), checkForError() and assigning of this.status are executed. Removal of deprecated events.

itotallyrock

First off, thank you for submitting a pull request.
So all around I like the changes, seems to maintain the current code quality while adding the new features. This update will likely bring with it several bugs that will need to be checked/tested for before merging. As for any specific issues I can think of would be the lack of documentation for the new events, changing the version (I think @JonnyBurger does this after merging), and several small code updates listed in this review.

itotallyrock · 2018-05-27T04:47:24Z

index.js

      if (isRunning) {
        resolve(true);
      } else {
-        reject(new Error('Cannot start Spotify.'));
+        reject(new Error('Cannot start Spotify Web Helper.'));


Good clarification on what process is actually unable to start.

itotallyrock · 2018-05-27T04:51:46Z

index.js

 function startSpotifyWebHelper() {
  return new Promise(function (resolve, reject) {
    var child = childProcess.spawn(getWebHelperPath(), { detached: true });
    child.on('error', function (err) {
      reject(new Error('Spotify is not installed. ' + err.message));
    });
    child.unref();
-    isSpotifyWebHelperRunning().then(function (isRunning) {
+    processStatus.of('SpotifyWebHelper').then(function (isRunning) {


We may want 'SpotifiyWebHelper' to be a constant.

Perhaps even export it through processStatus to help consistency, while offering the most flexibility.

itotallyrock · 2018-05-27T04:55:21Z

index.js

@@ -135,6 +100,13 @@ function SpotifyWebHelper(opts) {

  opts = opts || {};
  var localPort = opts.port || START_HTTPS_PORT;
+  var intervals = opts.intervals || {


Maybe use Object.assign here to avoid object referencing and better handling of default values.

var intervals = Object.assign({}, { checkIsRunning: 5000, checkIsShutdown: 2000, delayAfterStart: 3000, delayAfterError: 5000, retryHelperConnection: 5000 }, opts.intervals);

This will make sure all values of the interval option object are assigned unless overwritten.

itotallyrock · 2018-05-27T05:07:36Z

index.js

-        .catch(function (err) {
-          reject(err);
-        });
+        .catch(reject);


Good simplification here.

itotallyrock · 2018-05-27T05:32:39Z

index.js

    if (status.error) {
+      if (status.error.message === 'Invalid OAuth token') {
+        // TODO: It would be great to have something more precise than just the message to check.


This method of error handling is acceptable, if the status objects include an error code that would be more suitable, but they don't (I don't believe they do at least). Maybe we can add other error checks here to better handle them as well.

itotallyrock · 2018-05-27T05:35:50Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "spotify-web-helper",
-  "version": "1.14.0",
+  "version": "2.0.0",


The changes here are significant; however, unless they actually force some important new requirement or other end user changes (which they might I couldn't tell the difference), updating the major version number could be avoided to allow users to continue to use this repo with the new features without changing their package files.

Actually, in hindsight I do agree with the major version as anyone who would want to benefit from the open and close events will already be rewriting their code. That being said, we may want to group this together with some other changes (improved testing, debug logs in the whole project, etc.) to make it really major version worthy.

s-h-a-d-o-w · 2018-05-27T12:03:10Z

Thanks for the feedback, those are some great notes!

As for the README - that's actually one thing I wanted to leave up to you guys, since I find it sort of a "managerial decision" what to expose to the user via documentation. Personally, I'm all for transparency, so I'd e.g. document the intervals. But other people choose not to go into that much detail about all the options users could take advantage of...
Please let me know if you still want me to do it.

unless they actually force some important new requirement or other end user changes (which they might I couldn't tell the difference), updating the major version number could be avoided

Like I described - imagine someone constructed their apps around specific error events, handling starting and shutting down of Spotify themselves. If this is published as 1.x, they might update thinking "Ah, cool some patches/features" but it'll instead break their app, since the errors they rely on will simply not occur any more.

to make it really major version worthy

Oh don't get me started. ;)
There are a number of things I wanted to submit in a separate PR after this got merged, since I wanted this PR to be mostly about the new events.

Create an .editorconfig, since Webstorm was driving me absolutely nuts with indentation.
Unify white space. The test file had tabs (which I prefer myself) but index.js spaces.
Change to const/let variables throughout all of the project.
Think about and possibly adjust the declaration order of functions and white space between them.

I could do all of that because that's probably done in 1-2 hours.
However, I unfortunately do not have the time to create tests and if there is more debug output you want, I would suggest you adding in what you find adequate yourself because the way it is seems quite alright to me. The only thing I just thought might be handy is splitting debug levels into 'info' and 'verbose' and output the service responses on 'verbose'.

Anyway, my current todo list for this PR is:

Revert the major version change (Let @JonnyBurger handle that)
Fix specific issues you pointed out (Use of Object.assign(), constants, etc.)

Please let me know about:

How to handle documentation (edit README myself or if you want to do it after all)
How to handle the code refactoring I mentioned (separate PR or here)

…for intervals option Minor corrections regarding comments and unused variables.

itotallyrock · 2018-05-29T00:37:18Z

Good changes, nice use of Symbols as well. As for the readme, include everything to do with the new events, and if you want (strongly encouraged) include the new options you added. As for doing the code refactoring that should be a different pull request if you feel like tackling. Once you've added documentation its just up to what @JonnyBurger thinks.

s-h-a-d-o-w · 2018-05-29T11:05:27Z

While working on the README, I stumbled across something I found odd.
Based on the code, I would think that this.status is only updated whenever Spotify's service triggers an event (or every 60 seconds - whichever comes first).
So... how is it possible that if I read helper.status externally, the playing position actually changes? What am I missing here?

itotallyrock · 2018-05-29T18:04:59Z

If I'm understanding your question correctly, Here is the code for it, there is another interval it runs in. As a side note to this, it is an interval that could be configurable if you want to, you could add it to your intervals options somehow rather than a global const.

s-h-a-d-o-w · 2018-05-29T18:43:37Z

Oh wow, I didn't even see that - thanks! I've always assumed that all "seeking" has to do with playback control.
So if I understand it correctly, the playback_position is updated by us independently of the service and whenever a status from the service is received, that position is obviously overwritten.
Huh... well alright, I don't want to get into another thing here right now ;) but personally, rather than making that interval configurable, I would probably instead provide some getStatus() function that polls the service -> single source of truth.

In the example, I moved hooking up of events out of 'ready' because the user can obviously add even listeners whenever he wants.

So => debug output the ones we know will occur and emit the others. For instance, it was a bit annoying to figure out that there is a problem with packaging got 7.x: nexe/nexe#490

s-h-a-d-o-w added 3 commits May 24, 2018 21:35

Don't suppress errors, emit them. Also: customizable intervals for op…

5384d43

…erations Plus: Correct order in which compareStatus(), checkForError() and assigning of this.status are executed. Removal of deprecated events.

itotallyrock reviewed May 27, 2018

View reviewed changes

Revert on version; added Symbols for process-status; Object.assign() …

f74d2e4

…for intervals option Minor corrections regarding comments and unused variables.

s-h-a-d-o-w added 2 commits May 29, 2018 21:01

Updated docs regarding lifecycle events, intervals and debug output

2af5cbc

In the example, I moved hooking up of events out of 'ready' because the user can obviously add even listeners whenever he wants.

Suppressing errors during port checks can be problematic.

cf8b97d

So => debug output the ones we know will occur and emit the others. For instance, it was a bit annoying to figure out that there is a problem with packaging got 7.x: nexe/nexe#490

itotallyrock mentioned this pull request Jun 17, 2018

Excessive No user is logged in spam when Spotify is closed #23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #15 - Event for when Spotify is open #39

Fixes #15 - Event for when Spotify is open #39

s-h-a-d-o-w commented May 26, 2018

itotallyrock left a comment •

edited

itotallyrock May 27, 2018

itotallyrock May 27, 2018

itotallyrock May 27, 2018

itotallyrock May 27, 2018

itotallyrock May 27, 2018

itotallyrock May 27, 2018

itotallyrock May 27, 2018 •

edited

itotallyrock May 27, 2018

s-h-a-d-o-w commented May 27, 2018 •

edited

itotallyrock commented May 29, 2018

s-h-a-d-o-w commented May 29, 2018

itotallyrock commented May 29, 2018 •

edited

s-h-a-d-o-w commented May 29, 2018

Fixes #15 - Event for when Spotify is open #39

Are you sure you want to change the base?

Fixes #15 - Event for when Spotify is open #39

Conversation

s-h-a-d-o-w commented May 26, 2018

itotallyrock left a comment • edited

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

itotallyrock May 27, 2018 • edited

Choose a reason for hiding this comment

itotallyrock May 27, 2018

Choose a reason for hiding this comment

s-h-a-d-o-w commented May 27, 2018 • edited

itotallyrock commented May 29, 2018

s-h-a-d-o-w commented May 29, 2018

itotallyrock commented May 29, 2018 • edited

s-h-a-d-o-w commented May 29, 2018

itotallyrock left a comment •

edited

itotallyrock May 27, 2018 •

edited

s-h-a-d-o-w commented May 27, 2018 •

edited

itotallyrock commented May 29, 2018 •

edited