Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jul 9, 2022

Conversation

cmccambridge
Copy link
Contributor

@cmccambridge cmccambridge commented Jun 2, 2022

Implements #16

In this PR:

  • Find the rain delay active binary sensor to check whether currently under rain delay
  • Fetch the rain delay stop time sensor value and format as relative time into the secondary text
  • Use relativeTime formatting from custom-card-helpers (off-the-shelf clone of the HA frontend implementation)
  • Update custom-card-helpers dependency for current relativeTime implementation, and in turn update its dependency home-assistant-js-websocket.

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:

  • Verify that the upgrade of home-assistant-js-websocket is OK
    • Usage: Connection, createCollection in ha_entity_registry.ts, UnsubscribeFunc in opensprinkler-cards.ts
  • Wait for the updated HACS integration to be released so that this is easier to validate end to end
    • Released as 1.1.10

package.json Outdated Show resolved Hide resolved
src/helpers.ts Show resolved Hide resolved
@@ -209,6 +209,11 @@ export class OpensprinklerCard extends LitElement {
private _secondaryText() {
const entities: EntitiesFunc = p => this._matchingEntities(p)

if (hasRainDelayActive(entities)) {
const stop_time = entities(isRainDelayStopTime).find(_ => true)?.state;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise this feels like a hacky approach to selecting the first (and only expected) item in the list, without failing if the list is empty due to renames, etc. Good enough? Other ideas?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think entities(isRainDelayStopTime)[0]?.state would have the same effect. If there are no rain delay entities entities(isRainDelayStopTime)[0] will be undefined.

src/opensprinkler-card.ts Outdated Show resolved Hide resolved
- Use `relativeTime` formatting
- Update `custom-card-helpers` for current `relativeTime` implementation
@cmccambridge cmccambridge marked this pull request as draft June 2, 2022 04:56
@cmccambridge
Copy link
Contributor Author

cmccambridge commented Jun 2, 2022

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
Copy link
Contributor Author

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!

@cmccambridge cmccambridge marked this pull request as ready for review June 3, 2022 04:13
- Port relative_time from `custom-card-helpers` and `frontend` locally
- Adjust types to align minimum required locale fields across
  `HomeAssistant` object versions. (We require only language.)
- Take dependency on @formatjs/intl-utils for `selectUnit`, mirroring
  custom-card-helpers and frontend itself.
"@mdi/js": "^6.5.95",
"custom-card-helpers": "^1.8.0",
"custom-card-helpers": "1.8.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This is explicitly removing the "compatible with" ^ qualifier, in favor of pinning the custom-card-helpers version at exactly 1.8.0. Version 1.9.0 ought to be compatible, but took a major version bump of home-assistant-js-websocket, and we're exposed to the breaking changes due to data type incompatibility.

@rianadon
Copy link
Owner

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

@cmccambridge
Copy link
Contributor Author

No worries, thanks 😄

@rianadon
Copy link
Owner

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
Copy link
Owner

rianadon commented Jul 9, 2022

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: custom-cards/custom-card-helpers#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
Copy link
Owner

rianadon commented Jul 9, 2022

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

@rianadon rianadon merged commit af85c5c into rianadon:main Jul 9, 2022
@cmccambridge
Copy link
Contributor Author

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants