refactor(app-shell, app-shell-odd): Refactor app to use unsubscribe flags #14633
Conversation
…ing active subscription
| const { client } = connectionStore[hostname] | ||
| client?.unsubscribe(topic, {}, (error, result) => { | ||
| if (error != null) { | ||
| // log.warn( |
There was a problem hiding this comment.
might want this back but definitely don't want it commented - let's uncomment or delete
| }, RENDER_TIMEOUT) | ||
| }) | ||
| } else { | ||
| // log.info( |
| @@ -434,3 +391,14 @@ function sendToBrowserDeserialized({ | |||
|
|
|||
| browserWindow.webContents.send('notify', hostname, topic, deserializedMessage) | |||
| } | |||
|
|
|||
| function deserialize(message: string): string | Record<string, unknown> { | |||
There was a problem hiding this comment.
Is there a case where this will be (a) valid and (b) not json? if so can we remove that case, and make this be a Promise<Record<string, unknown>> or even better a Promise<NotifyMessage> with a small list of valid models that rejects if it doesn't parse?
|
Soooo, I borked this PR (but the feedback I'll carry over!). Turns out I manufactured the end result that assumes discovery-client handles the connection logic alongside the unsubscribe logic. That's what I get for doing too many things on too many branches at once. Correcting this PR without creating a complex intermediary solution that utilizes the backend unsubscribe flags seems pretty pointless, as does changing one variable on the app side that refetches on the unsubscribe flag. I'll just re-open a PR with the end result that includes the discovery-client logic. That way, the node layer actually makes use of the new unsubscribe flags. |
Closes EXEC-306
Overview
This is the frontend implementation of #14620. See that PR and its corresponding ticket for the explanation behind this refactor.
Test Plan
Risk assessment
low