search

custom-cards / custom-card-helpers

Custom Card Helper Functions/Types for Developers
https://custom-cards.github.io/custom-card-helpers/
MIT License
48 stars 29 forks source link

hass.locale is considered optional, but is required input for helper functions #42

Closed nielsfaber closed 2 years ago

nielsfaber commented 3 years ago

I would like to raise awareness for a problem which I addressed as well in https://github.com/custom-cards/custom-card-helpers/issues/35 (which was recently closed).

The HomeAssistant.locale property is considered optional (link). This breaks a simple call like:

computeStateDisplay(hass.localize, hass.states["switch.my_switch"], hass.locale)

Due to hass.locale being "possibly undefined".

My project is still depending on 1.7.0 of custom-card-helpers (dated february), since the broken function calls block me from updating.

How are we supposed to use the helper functions which require the hass.locale as input?

marksie1988 commented 2 years ago

Workaround in typescript is to do hass.locale!.xxx this will ignore the optional, but keep in mind that if someone uses an old ha version this will cause an error.

nielsfaber commented 2 years ago

I understand, but I was referring to a solution that would work on both old + new setups. I'm not really happy with this change, since it forces all custom-card owners to apply your workaround which in turn forces all users to run latest-greatest HA version, or the card crashes for them.

I think it is much better if for example the computeStateDisplay allows either hass.locale or hass.language as localization argument, so support for older setups can be maintained this way:

computeStateDisplay(hass.localize, hass.states["switch.my_switch"], hass.locale | hass.language)

We should be careful with introducing breaking changes, especially if a fallback is easy to implement...

JonahKr commented 2 years ago

Hey there @nielsfaber 👋, There was a discussion in the PR itself at the time (#30) about how to handle backward compatibility and the result was, since hass.language is already deprecated and will be removed completely in a few months, to not create a extractLocale function as a standardized way for assuring backwards compatibility. Additionally at the time of the PR the locale attribute was in home assistant for more than 2 months already. You are right though that this should have been communicated better.

Now there have been 6 Major releases in between and with each month there will be less people effected by this. -> https://analytics.home-assistant.io/

If you still think that this should be tackled I am all for it but it should be thought about if its worth the effort.

Take Care! 👍

nielsfaber commented 2 years ago

@JonahKr Thanks for taking the effort of responding to my issue! Agreed that this is becoming less of an issue over time. I have no idea about how many users are running a setup which is more than 6 months old (and thus may lack the hass.locale property). According to the HA analytics the portion is approx. 5% (but then again, analytics was introduced around the same time, so its a bit of a chicken-and-egg problem here).

Anyway, I'm OK with moving forward 👍 But it means that https://github.com/custom-cards/custom-card-helpers/blob/93f292ae2e51640aea20ff59fc121e43651cf463/src/types.ts#L210

Should be not longer be an optional parameter, such that the hass.locale parameter can be passed in calls such as computeStateDisplay.

Otherwise, how am I supposed to call this function from my custom-card? The only workaround I see is mentioned in https://github.com/custom-cards/custom-card-helpers/issues/42#issuecomment-921271729, but this shouldn't be the recommended way of using it, right? Could you share some example of the intended way of calling such function?

Thanks for your time.

JonahKr commented 2 years ago

According to the HA analytics the portion is approx. 5% (but then again, analytics was introduced around the same time, so its a bit of a chicken-and-egg problem here).

Yes this is true ofc. But i still think that you can expect your users to update Home-Assistant at least once or twice a year and if they want a new version of a custom card, they will have to run a somewhat up to date version if thats required. You can even define minimum required versions for Hacs installables at least.

Should be not longer be an optional parameter

Thats true. I will change it when i am updating the time functions. 👍

The thing @ marksie1988 pointed out using hass.locale! is perfectly valid and only says that you are sure that the locale attribute exists on the hass object.