search

svrooij / node-sonos-ts

:speaker: Sonos control library, use this library in your own appliction.
https://sonos-ts.svrooij.io/
MIT License
81 stars 18 forks source link

#147 fetch error #148

Closed theimo1221 closed 2 years ago

theimo1221 commented 2 years ago

Harden Play Notification Queue

Description

Harden playNotificationQueue by:

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

💔 Thank you!

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1584680187

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/sonos-device.ts 39 64 60.94%
<!-- Total: 39 64 60.94% -->
Totals Coverage Status
Change from base Build 1306852614: -0.6%
Covered Lines: 1482
Relevant Lines: 1919
💛 - Coveralls
svrooij commented 2 years ago

I'm ok with you changing the code for the second notification queue. I'm not comfortable with you changing stuff to the base service. My guess it that everything could be fixed with a try { .... } catch (e) { ... }.

Or if you want to keep close to the callbacks, you can add .catch((err) => { .... }); after each async call. Like I do here: https://github.com/svrooij/node-sonos-ts/blob/a4abbca1fe4203bc9bcfa40d4e7ef9172ac1abd5/src/sonos-device.ts#L394-L397

One of the things of using async it that there can be errors, with callbacks you had to return them in the second argument, without callbacks you just throw an error.

I cannot find a single reason why catchEm.... part is needed while .catch((err) => { ... } is supported by default.

theimo1221 commented 2 years ago

I'm ok with you changing the code for the second notification queue. I'm not comfortable with you changing stuff to the base service. My guess it that everything could be fixed with a try { .... } catch (e) { ... }.

Okay, I reverted the changes to base-service.ts 👍

I cannot find a single reason why catchEm.... part is needed while .catch((err) => { ... } is supported by default.

This had partially to do with DeepSource and EsLint:

// Invalid for DeepSource "Missing initialization during variable declarations JS-0309"
let originalPositionInfo: GetPositionInfoResponse | undefined;

// Invalid for EsLint "'no-undef-init'"
let originalPositionInfo: GetPositionInfoResponse | undefined = undefined;

If it is okay for you to set 'no-undef-init': 'off', we can do it without async helper.

svrooij commented 2 years ago

@theimo1221 I didn't read your comment before responding. Deepsource is just an experiment to try to make the library better. You can ignore the results from deepsource. Following eslint in this case.

Just an observation, if I run the play-notification.js example, it plays all 3 notifications after each other. If I change the PlayNotification and PlayTTS occurrences to their ...Two version and try to re-run the example it only plays one notification and doesn't revert back to my christmas radio station.

Can you create a sample file that calls your methods that uses setTimeout to send a second notification while the first one is still playing. That way everybody can verify the intended behavior.

theimo1221 commented 2 years ago

Good Morning @svrooij,

good Idea! I just added an example, which works with my devices in both situations (Radio running before start + Reverting to radio afterwards, nothing playing).

Do you want me to revert that 'no-undef-init': 'off',. If so please turn off that DeepSource rule.

Thanks and best regards

svrooij commented 2 years ago

I already turned JS-0309 off in DeepSource 😉, any is still not allowed.

My understanding was that you wanted to do something like this, (have the PlayNotificationTwo be awaitable, and resolve when that specific notification played).:

playNotification('https://cdn.smartersoft-group.com/various/someone-at-the-door.mp3', 2000).then((played) => {
  console.log('Notification 1 played')
});
playNotification('https://cdn.smartersoft-group.com/various/pull-bell-short.mp3', 2500).then((played) => {
  console.log('Notification 2 played')
});

This is not in the sample, does that work though? Because that would be the main reason to use your version instead of the other one. At the moment both samples seem to accomplish the exact same thing.

theimo1221 commented 2 years ago

I already turned JS-0309 off in DeepSource 😉, any is still not allowed.

Okay, reverted eslint change and updated code accordingly

This is not in the sample, does that work though? Because that would be the main reason to use your version instead of the other one. At the moment both samples seem to accomplish the exact same thing.

Yes, it works like that --> I updated example to show that as well.

svrooij commented 2 years ago

I cannot edit your version, can you pull this branch https://github.com/svrooij/node-sonos-ts/tree/%23147-FetchError ? It moves the notification options to the notification queue file. After that I'll accept the changes to the beta branch. One more question, what is the use of catchQueueErrors? Should it not always catch the errors when working with the notification queue?

Debugging

If you create a .env file in the root with the following content, you can just press F5 in VSCode to run any example with debugging active (eg. breakpoints):

DEBUG=sonos:*
SONOS_HOST=192.168.178.41
# SONOS_TTS_ENDPOINT=https://xyz.xyz.io/api/generate
# SONOS_TTS_LANG=nl-NL

If you then use const sonos = new SonosDevice(process.env.SONOS_HOST) in a sample, it should pick up the correct IP, from the env file.

theimo1221 commented 2 years ago

Good evening @svrooij,

Thanks for the .env information and your review. I'm sorry I guess I forgot to approve changes by author... --> I merged your updated branch.

Have a nice Evening 👍

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.5.0-beta.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 2.5.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: