Skip to content

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

Merged
balloob merged 4 commits intohome-assistant:devfrom
bdraco:shutdown_restart_never_expected_to_return
Apr 17, 2021
Merged

Do not throw warnings when a service calls disconnects the websocket#8932
balloob merged 4 commits intohome-assistant:devfrom
bdraco:shutdown_restart_never_expected_to_return

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Apr 17, 2021

Proposed change

Do not throw warnings when a service calls disconnects the websocket

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

bdraco added a commit to bdraco/home-assistant that referenced this pull request Apr 17, 2021
- Waiting was unreliable since the websocket response could take a few seconds to get delivered
- Alternate frontend fix is home-assistant/frontend#8932
balloob pushed a commit to home-assistant/core that referenced this pull request Apr 17, 2021
…49323)

- Waiting was unreliable since the websocket response could take a few seconds to get delivered
- Alternate frontend fix is home-assistant/frontend#8932
Comment on lines +283 to +286
const [domain, service] = this._serviceData.service.split(".", 2);
if (
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(domain, service)
Copy link
Copy Markdown
Member

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)

@balloob balloob merged commit 5199883 into home-assistant:dev Apr 17, 2021
err.error.code === ERR_CONNECTION_LOST &&
serviceCallWillDisconnect(domain, service)
) {
throw err;
Copy link
Copy Markdown
Member

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

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

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

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

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

Choose a reason for hiding this comment

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

Thanks! and Good night!

@github-actions github-actions Bot locked and limited conversation to collaborators Apr 18, 2021
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.

4 participants