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

Get persistent_notifications for lovelace from websocket. #1649

Merged
merged 3 commits into from
Sep 17, 2018

Conversation

jeradM
Copy link
Member

@jeradM jeradM commented Sep 10, 2018

Frontend changes for home-assistant/core#16503

Lovelace will now use websocket connection and events to fetch/update persistent_notifications

@@ -260,6 +288,7 @@ class HUIRoot extends NavigateMixin(EventsMixin(PolymerElement)) {
_hassChanged(hass) {
if (!this.$.view.lastChild) return;
this.$.view.lastChild.hass = hass;
this._updateNotifications();
Copy link
Member

Choose a reason for hiding this comment

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

This is very expensive. This will now query the websocket backend whenever hass changes, which is A LOT. We should cache the websocket response in a different variable and only update that on the persistent_notifications_updated event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, that was a bad idea

@@ -165,6 +178,33 @@ class HUIRoot extends NavigateMixin(EventsMixin(PolymerElement)) {
this._debouncedConfigChanged = debounce(() => this._selectView(this._curView), 100);
}

async connectedCallback() {
super.connectedCallback();
this._fetchPersistentNotifications();
Copy link
Member

Choose a reason for hiding this comment

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

Ha, just realized, we should make this a collection ! That way any location can subscribe to it and it will always stay in sync. Example of collection is here

Docs here. Make sure the first argument to createCollection is unique.

Copy link
Member

Choose a reason for hiding this comment

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

Collections will also fetch again after a reconnect.

Copy link
Member

Choose a reason for hiding this comment

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

The return value of a collection is still an unSub function that should be called when the component disconnects.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Do we need to change the backend so that it sends the updated data with the event?

Copy link
Member

Choose a reason for hiding this comment

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

No, then you need to get granular events like state_changed and only include the changed pieces.

Copy link
Member

Choose a reason for hiding this comment

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

So do something like this in subscribeUpdates:

const fetchNotifications = conn => conn.sendMessagePromise({
  type: 'persistent_notification/get'
});

const subscribeUpdates = (conn, store) =>
  conn.subscribeEvents(
    event => fetchNotifications(conn).then(notif => store.setState(notif, true)),
    'persistent_notifications_updated'
  );

export const subscribeNotifications = (conn, onChange) =>
  createCollection(
    '_ntf',
    fetchNotifications,
    subscribeUpdates,
    conn,
    onChange
  );

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Will do. Later I can add more fine-grained events so we can update the collection without having to fetch the whole set

Copy link
Member

Choose a reason for hiding this comment

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

I honestly think that it would be overkill. It should be such small data sets.

value: []
},

notifications: {
Copy link
Member

Choose a reason for hiding this comment

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

_notifications

@@ -1,8 +1,7 @@
import computeDomain from '../../../common/entity/compute_domain.js';

const NOTIFICATION_DOMAINS = [
'configurator',
'persistent_notification'
'configurator'
];

export default function computeNotifications(states) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that it's only 1 domain, it's faster to do === instead of includes

@balloob
Copy link
Member

balloob commented Sep 12, 2018

We can merge this once we are no longer needing to do frontend updates for the beta.

@balloob balloob merged commit 5187f3b into home-assistant:master Sep 17, 2018
@ghost ghost removed the in progress label Sep 17, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants