Skip to content

Do not throw warnings when a service calls disconnects the websocket #8932

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.

Sign up for GitHub

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

Merged
balloob merged 4 commits into from
Apr 17, 2021
Merged

Do not throw warnings when a service calls disconnects the websocket #8932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
575b32b
Do not throw warnings when a service calls disconnects the websocket
bdraco Apr 16, 2021
2caed57
tsc
bdraco Apr 17, 2021
4835756
its js
bdraco Apr 17, 2021
4979587
tsc
bdraco Apr 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/data/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ export const callExecuteScript = (hass: HomeAssistant, sequence: Action[]) =>
type: "execute_script",
sequence,
});

export const serviceCallWillDisconnect = (domain: string, service: string) =>
domain === "homeassistant" && ["restart", "stop"].includes(service);
15 changes: 14 additions & 1 deletion src/panels/developer-tools/service/developer-tools-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ERR_CONNECTION_LOST } from "home-assistant-js-websocket";
import { safeLoad } from "js-yaml";
import {
css,
Expand All @@ -23,7 +24,11 @@ import "../../../components/ha-service-picker";
import "../../../components/ha-yaml-editor";
import type { HaYamlEditor } from "../../../components/ha-yaml-editor";
import { ServiceAction } from "../../../data/script";
import { callExecuteScript } from "../../../data/service";
import { forwardHaptic } from "../../../data/haptics";
import {
callExecuteScript,
serviceCallWillDisconnect,
} from "../../../data/service";
import { haStyle } from "../../../resources/styles";
import "../../../styles/polymer-ha-style";
import { HomeAssistant } from "../../../types";
Expand Down Expand Up @@ -275,6 +280,14 @@ class HaPanelDevService extends LitElement {
try {
await callExecuteScript(this.hass, [this._serviceData]);
} catch (err) {
const [domain, service] = this._serviceData.service.split(".", 2);
if (
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(domain, service)
Comment on lines +283 to +286
Copy link
Copy Markdown
Member

@balloob balloob Apr 17, 2021

Choose a reason for hiding this comment

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

This can also work:

Suggested change
const [domain, service] = this._serviceData.service.split(".", 2);
if (
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(domain, service)
if (
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(...this._serviceData.service.split(".", 2))

(but is less readable)

) {
return;
}
forwardHaptic("failure");
showToast(this, {
message:
this.hass.localize(
Expand Down
8 changes: 8 additions & 0 deletions src/state/connection-mixin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
callService,
Connection,
ERR_INVALID_AUTH,
ERR_CONNECTION_LOST,
HassConfig,
subscribeConfig,
subscribeEntities,
Expand All @@ -13,6 +14,7 @@ import { broadcastConnectionStatus } from "../data/connection-status";
import { subscribeFrontendUserData } from "../data/frontend";
import { forwardHaptic } from "../data/haptics";
import { DEFAULT_PANEL } from "../data/panel";
import { serviceCallWillDisconnect } from "../data/service";
import { NumberFormat } from "../data/translation";
import { subscribePanels } from "../data/ws-panels";
import { translationMetadata } from "../resources/translations-metadata";
Expand Down Expand Up @@ -78,6 +80,12 @@ export const connectionMixin = <T extends Constructor<HassBaseEl>>(
target
)) as Promise<ServiceCallResponse>;
} catch (err) {
if (
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(domain, service)
) {
throw err;
Copy link
Copy Markdown
Member

@bramkragten bramkragten Apr 17, 2021

Choose a reason for hiding this comment

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

Throw? Not return?

Copy link
Copy Markdown
Member Author

@bdraco bdraco Apr 17, 2021

Choose a reason for hiding this comment

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

I wasn't sure how all the existing callers would handle it if they got back nothing instead of the promise so throw + suppressing the notification in the UI seemed like the way to go.

Better ideas?

Copy link
Copy Markdown
Member

@bramkragten bramkragten Apr 17, 2021

Choose a reason for hiding this comment

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

I think almost nothing in the frontend uses the return value of callService, I introduced it for the scenes editor a year ago, if they do, Typescript should be able to tell you the issues :-)

If we throw, it will not be caught by the caller, as we previously never thrown errors, but caught errors here. I would return undefined to keep current behavior.

Copy link
Copy Markdown
Member

@bramkragten bramkragten Apr 17, 2021

Choose a reason for hiding this comment

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

(and move it after the dev console log)

Copy link
Copy Markdown
Member Author

@bdraco bdraco Apr 17, 2021

Choose a reason for hiding this comment

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

done here #8937

Will check it in the morning.

late here. g'night

Copy link
Copy Markdown
Member

@bramkragten bramkragten Apr 17, 2021

Choose a reason for hiding this comment

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

Thanks! and Good night!

}
if (__DEV__) {
// eslint-disable-next-line no-console
console.error(
Expand Down