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

Add ability for browser to receiver updates to already discovered services. #19

Merged
merged 9 commits into from
Mar 24, 2024

Conversation

NorthernMan54
Copy link

I was looking at adding the ability to automatically detect changes to accessories and bridges, and needed the capability to receive TXT record updates. The existing Browser implementation just ignores them, so this pull request adds another event that will emit when a new message is received for an already discovered service.

This change is 100% backwards compatible with existing implementations.

@NorthernMan54 NorthernMan54 requested a review from Supereg March 21, 2024 20:23
@coveralls
Copy link

coveralls commented Mar 21, 2024

Pull Request Test Coverage Report for Build 8402488019

Details

  • 12 of 13 (92.31%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 82.665%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/Browser.js 12 13 92.31%
Totals Coverage Status
Change from base Build 7907585379: -0.07%
Covered Lines: 367
Relevant Lines: 405

💛 - Coveralls

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks for the addition.

EDIT: Oops, forgot some comments, see final comment below.

lib/Browser.js Outdated Show resolved Hide resolved
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks for the addition.

Just had a smaller comment and would love to request the addition of some unit test covering this functionality. Once that is implemented I'm happy to have this PR merged 🚀

lib/Browser.js Show resolved Hide resolved
@NorthernMan54 NorthernMan54 changed the title Add ability to for browser to receiver updates to already discovery services. Add ability for browser to receiver updates to already discovered services. Mar 23, 2024
@NorthernMan54
Copy link
Author

NorthernMan54 commented Mar 23, 2024

@Supereg Before merging this, I think I need to increment the version in package etc. Does it auto publish to NPM or is it manual ?

FYI - I have been attempting to record the publishing steps for each package here - https://github.com/homebridge/documentation/blob/latest/Repositories.md

@Supereg
Copy link
Member

Supereg commented Mar 24, 2024

@Supereg Before merging this, I think I need to increment the version in package etc. Does it auto publish to NPM or is it manual ?

FYI - I have been attempting to record the publishing steps for each package here - https://github.com/homebridge/documentation/blob/latest/Repositories.md

It will auto publish if you push a new version tag/commit. So just merge and I will publish for you.
not ideal for people without permissions to publish to main. We could aim for creating a workflow that does that version commit and tag eventually.

@NorthernMan54 NorthernMan54 merged commit 07f5631 into homebridge:master Mar 24, 2024
8 checks passed
@NorthernMan54
Copy link
Author

@Supereg Tks Merged

@Supereg
Copy link
Member

Supereg commented Mar 25, 2024

Just double checked, I thought there was automatic publishing configured for this repository. It's not. It's currently manual. So feel free to create an issue for that. I'm here to help if someone wants to set something up 👍

@NorthernMan54
Copy link
Author

#20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants