-
Notifications
You must be signed in to change notification settings - Fork 59
feat(toolkit-lib): network detector #926
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
base: main
Are you sure you want to change the base?
Conversation
| // Check connectivity before attempting network request | ||
| const hasConnectivity = await NetworkDetector.hasConnectivity(this.agent); | ||
| if (!hasConnectivity) { | ||
| throw new ToolkitError('No internet connectivity detected'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is throwing the right thing here? Is that error caught elsewhere? Asking because Notices should just silently fail. A comment might help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the right thing to do here. we are throwing errors in web-data-source on failures and expecting to swallow them elsewhere (which we do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] Since this pattern will be very common (get the result, check whether it's true and throw an error if not), we could also have a method that takes a callback and does this for you:
return NetworkDetector.ifConnected(async() => {...});There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good thought and i have considered this. im not entirely against it, but i feel like my more (naive) approach works more intuitively even if it reuses the same pattern. we can always refactor in the future if it turns out that ifConnected is a cleaner API
| ); | ||
| }); | ||
|
|
||
| test('skips request when no connectivity detected', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the true result: we do not ping the telemetry endpoint in environments without internet access
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
- Coverage 84.17% 83.60% -0.57%
==========================================
Files 71 71
Lines 10437 10445 +8
Branches 1334 1324 -10
==========================================
- Hits 8785 8733 -52
- Misses 1611 1673 +62
+ Partials 41 39 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: github-actions <github-actions@github.com>
Signed-off-by: github-actions <github-actions@github.com>
this PR implements a connectivity detector that performs a HEAD request to the notices endpoint, with a timeout of 1 hour. before making any subsequent network calls, (including GET notices and PUT telemetry) we check to see if we have already determined the network to be disconnected. in those cases, we skip subsequent calls.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license