-
Notifications
You must be signed in to change notification settings - Fork 295
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
Use fetch instead of apiFetch. #9936
Use fetch instead of apiFetch. #9936
Conversation
Build files for 3cded87 have been deleted. |
Size Change: +54 B (0%) Total Size: 1.98 MB
ℹ️ View Unchanged
|
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.
Thanks @ankitrox – this looks very close, just one suggestion about the way the URL is constructed.
Thanks @aaemnnosttv for reviewing the PR and providing the feedback. I've made the suggested changes and assigning it to you for another review. |
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.
Thanks @ankitrox – after some reconsideration, I think we should slightly revise the approach to avoid reaching for the API fetch data.
// We are only interested if the request was successful, to | ||
// confirm online status. | ||
const canReachConnectionCheck = !! connectionCheckResponse; | ||
await fetch( new URL( '/google-site-kit/v1/', rootURL ) ); |
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.
After some more thought here, I think this is not quite the right solution (criticizing my own IB here! 😄).
The fact that we're reaching for some internal api-fetch data here is a smell that we should be using that instead. The reason I suggested using fetch
instead was to avoid the additional cases where api-fetch
throws, where fetch
only throws when the request fails. Essentially we should be able to do the same using api-fetch
with an additional check in the throw block as api-fetch
returns a specific error in this case. See https://github.com/WordPress/gutenberg/blob/d9b726b8451746703cc1b9680487e3726ab4a03f/packages/api-fetch/src/index.js#L130-L131
In summary, we can keep most of what we had before, but adjusted to use an approach closer to the suggested one here in letting the try/catch determine the result. The difference in using api-fetch
is that we need to set the online state conditionally depending on the error code if it throws. I.e. if it throws and the error code is fetch_error
, only then should we set it to be offline. Any other error would mean the request happened, which is all we are concerned about here, and can be ignored.
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.
Thanks @aaemnnosttv , I've made the changes according to the suggestion.
…b.com:google/site-kit-wp into infrastructure/9485-periodic-connection-check.
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.
Thanks @ankitrox – this can be further simplified a little bit which I'll do now before merging, otherwise LGTM!
@@ -63,7 +63,12 @@ export function useMonitorInternetConnection() { | |||
|
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.
We can remove all of the response checking above because we're only concerned with it throwing or not.
E2E failures are unrelated. See #8229 |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist