Replace "getStates" by more efficient entities subscription (compressed states) #49
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
- Coverage 99.93% 99.87% -0.06%
==========================================
Files 46 50 +4
Lines 1499 1593 +94
==========================================
+ Hits 1498 1591 +93
- Misses 1 2 +1 ☔ View full report in Codecov by Sentry. |
Source/Caches/HACacheKeyStates.swift
Outdated
| /// Logic from: https://github.com/home-assistant/home-assistant-js-websocket/blob/master/lib/entities.ts | ||
| static func processUpdates(info: HACacheTransformInfo<HACompressedStatesUpdates, HACachedStates?>) | ||
| -> HACachedStates { | ||
| var states = info.current ?? .init(entities: []) |
There was a problem hiding this comment.
What happens if we're starting a new subscription and we get back the 'initial' value set. Do we handle the case where: old value contains entity X, new subscription's first result doesn't include X - does X continue to show? I think it does from how this code path works.
There was a problem hiding this comment.
This may be a reason to 'populate' (but always set the value to nil) the cache for this particular state, which might require changes to the other PR, too. Or perhaps a flag to let it know, in this case, that it's coming from stale data. Hmm.
There was a problem hiding this comment.
If X is not included in the second update it's going to be removed by the "processUpdate" right? during the "subtractions" check on line 41
In the end this should be almost instant, all these changes are just to accommodate the fact that the new entities subscription sends a message in between the call and sending the entities state
There was a problem hiding this comment.
If the old value is from an hour ago, the initial subscription isn't going to include it as a deletion because it doesn't know you want to delete it.
Source/Caches/HACacheKeyStates.swift
Outdated
| do { | ||
| states[entityId] = try updates.asEntity(entityId: entityId) | ||
| } catch { | ||
| HAGlobal.log(.error, "Failed adding new entity: \(error)") |
There was a problem hiding this comment.
You should still invoke this case in tests, if you can.
There was a problem hiding this comment.
I did a little workaround temporaraly implementing logs and then resetting it, let me know what you think
Source/Data/HACompressedEntity.swift
Outdated
| } | ||
| } | ||
|
|
||
| public struct CompressedEntityDiff: HADataDecodable { |
There was a problem hiding this comment.
Do many of these need to be public?
Also: prefix
There was a problem hiding this comment.
Yes because HACompressedStatesUpdates needs to be public so HATypedSubscription.subscribeEntities can also be public
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
bgoncal
force-pushed
the
subscribe-without-populate
branch
from
2fd57b6 to
f682702
Compare
bgoncal
force-pushed
the
new-entities-subscription-2
branch
9 times, most recently
from
6f15a35 to
046e64e
Compare
bgoncal
force-pushed
the
new-entities-subscription-2
branch
from
046e64e to
335db2d
Compare
bgoncal
force-pushed
the
new-entities-subscription-2
branch
from
ebcc786 to
4a57ed1
Compare
bgoncal
force-pushed
the
new-entities-subscription-2
branch
from
4a57ed1 to
26c5f37
Compare
This depends on #48