rianadon / opensprinkler-card

Home Assistant card for collecting OpenSprinkler status
Other
66 stars 8 forks source link

Show Rain Delay Stop Time if rain delay is active #17

Closed cmccambridge closed 2 years ago

cmccambridge commented 2 years ago

Implements #16

In this PR:

Net Result: 2 hour rain delay: image 28 hour rain delay: image

Note: proper functionality here is dependent on a bugfix from the HACS opensprinkler integration being published, which corrects an issue on the integration side where the rain delay stop time was not being properly converted from local time to UTC. That fix is now merged and on its way through the release process, but without it the reported rain delay times may be surprising πŸ˜„

TODO:

cmccambridge commented 2 years ago

Converting to a draft for the moment... I somehow snuck through successful local testing in between npm runs, but starting from scratch shows that the custom-card-helpers upgrade propagates more deeply... This change would (i think?) require updating custom-card-helpers in lovelace-timer-bar-card as well, in order to propagate our HomeAssistant instance from one card to the next...

It's possible that my newbie level JavaScript (and TypeScript) is simply missing some trick here, but at the moment I get errors like:

Error: /workspaces/opensprinkler-card/src/opensprinkler-control.ts(62,23): semantic error TS2345: Argument of type 'import("/workspaces/opensprinkler-card/node_modules/custom-card-helpers/dist/types").HomeAssistant' is not assignable to parameter of type 'import("/workspaces/opensprinkler-card/node_modules/lovelace-timer-bar-card/node_modules/custom-card-helpers/dist/types").HomeAssistant'.
  Types of property 'auth' are incompatible.
    Type 'import("/workspaces/opensprinkler-card/node_modules/custom-card-helpers/node_modules/home-assistant-js-websocket/dist/auth").Auth' is not assignable to type 'import("/workspaces/opensprinkler-card/node_modules/lovelace-timer-bar-card/node_modules/home-assistant-js-websocket/dist/auth").Auth'.
      Types have separate declarations of a private property '_saveTokens'.

@rianadon Is it preferable here to propagate this upgrade onward into the timer bar card as well? Or, should I drop the custom-card-helpers dependency back down to 1.8.0 and sort out the relative time some other way?

The hang up on 1.8.0 is that something is going sideways in localize and I end up with empty strings out the far end of relativeTime. Others have reported that issue, but it's fixed with the move to locale data in 1.9.0.

cmccambridge commented 2 years ago

OK, in the interest of driving at least one of the possible solutions all the way to ground, here's one feature complete PR approach :)

This follows the lead of custom-card-helpers by copy-pasting the relativeTime routine out of Home Assistant frontend. However, it does not attempt to actually update opensprinkler-card's dependency on custom-card-helpers from 1.8.0 to 1.9.0 due to the propagating data type changes that would be required even out to the lovelace-timer-bar-card dependency.

This PR takes a dependency on @formatjs/intl-utils, the package I called out in #16 as being deprecated... However, two further thoughts on that, digging deeper:

  1. Home Assistant frontend and custom-card-helpers still take this same dependency, so it's not net new for the user.
  2. Upgrading the opensprinkler-card to custom-card-helpers 1.9.0 or higher in future will realign the necessary data types so that custom-card-helpers's copy of relativeTime can be used directly (i.e. delete the copy in opensprinkler-card. At that point, managing this dependency will be offloaded to the shared helpers package, which is a good thing πŸ‘

Let me know your thoughts :) This one is actually functional for me, and I still think a nice improvement πŸ˜„

Happy to do any linting, commit squashing etc that you prefer- just let me know!

rianadon commented 2 years ago

Sorry for the delay. I'll try to take a look by the end of this month.

cmccambridge commented 2 years ago

No worries, thanks πŸ˜„

rianadon commented 2 years ago

Wow thanks for all the writeups! The information is super helpful. I remember there being some apis that were changed in custom-card-helpers that prevented me from upgrading it in the timer card. Stuff might have changed in the meantime though so I'll take another look. Based on what you said, it seems like upgrading is the cleanest solution.

rianadon commented 2 years ago

Sorry it took so long to find time to look into this. The reason I can't upgrade custom-card-helpers in the timer bar card is this issue: https://github.com/custom-cards/custom-card-helpers/issues/55. It looks like custom-card-helpers has been unmaintained since January so I'm not expecting the issue to be fixed.

I think maintaining our own relative_time.ts is a great idea. Especially since @formatjs/intl-utils is deprecated: This way when Home Assistant replaces the library in their frontend, we can easily update our relative_time.ts to match.

rianadon commented 2 years ago

The code looks spectacular to me so I'll do the merge. Thank you so much for the PR and comments @cmccambridge!

cmccambridge commented 2 years ago

Thanks very much @rianadon! It was fun to dive into this card to add this new ability in. Thanks for the pointers and feedback along the way πŸ˜„