api-fetch: Check navigator.onLine to improve failure notices #71438
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| ! isPublished && publishStatus.indexOf( edits.status ) !== -1 | ||
| ! isPublished && edits.status in messages | ||
| ? messages[ edits.status ] | ||
| : __( 'Updating failed.' ); |
There was a problem hiding this comment.
Nit: let's move this "Updating failed" to a messages.default field. So that the code the two branches (offline_error or not) uses the same pattern.
There was a problem hiding this comment.
Right, I meant to and then forgot :)
packages/api-fetch/src/index.js
Outdated
| if ( ! window.navigator.onLine ) { | ||
| throw { | ||
| code: 'offline_error', | ||
| message: __( 'You are offline.' ), |
There was a problem hiding this comment.
I'm thinking whether "Your network connection is offline" would be more polite. Telling someone "You are " is a bit blunt 🙂
There was a problem hiding this comment.
Technically, a network connection isn't offline, the device is, right? Does it make more sense to make it "Your device is offline" or "The device is offline"?
If we're aiming for more politeness while not sacrificing any accuracy, we could also do one of those:
- You're not connected to the internet.
- We’re having trouble connecting — please check your network connection.
- No internet connection detected.
- Your device appears to be offline.
- Unable to connect. Please check your internet connection.
There was a problem hiding this comment.
Thanks, I can see how it would seem blunt (although this message shouldn't surface to the user, in practice). I had also started with something like "device is offline", but I thought it would sound confusing or artificial for most users; similarly, "network is down / not connected to the network" could be a little technical-sounding. But I like some of the suggestions there. My favourite ones:
- You're not connected to the internet.
- Your device appears to be offline.
- Unable to connect. Please check your internet connection.
packages/api-fetch/src/index.js
Outdated
| throw { | ||
| code: 'fetch_error', | ||
| message: __( 'You are probably offline.' ), | ||
| message: __( 'An unexpected error occurred.' ), |
There was a problem hiding this comment.
I'd make this error more specific, mentioning there was something wrong with a network request: "Network request failed." The problem is that when the error is displayed in an error notice, there is no context from which the user could know that it was a network error that happened.
There was a problem hiding this comment.
@jsnajdr doesn't this happen when the error is not a connection issue? With that in mind, is it possible "network request failed" could hint to the wrong problem, and the user would think it's a network connection issue?
There was a problem hiding this comment.
doesn't this happen when the error is not a connection issue?
Correct. But I think the suggestion still holds, because strictly speaking it was a request that failed, for whatever reason (not just the connection).
I think we can try "network request failed", no strong opinion here. What I think is also worth doing (but I wanted to keep it in a separate PR) is adding a layer that "sniffs" the error message for likely connection errors. That would mean that this fallback message would be shown even less frequently.
There was a problem hiding this comment.
When fetch fails with the constant generic error, like Failed to fetch in Chrome, it's either a problem with network or inability of the server to read a HTTP request and return a HTTP response (even a 5xx one). In my enumeration of possible errors in #67141 (comment), it's error 3 (DNS) and the following ones.
I think it's not wrong to call them "network errors". They are not necessarily problems with your network connection, but network errors they are.
There was a problem hiding this comment.
While I agree with you both, I still think that the nuance between "network error, but not a connection error" and "network connection error" can still be confusing to the user and we should try to make it clearer for non-technical people who don't necessarily make a difference.
There was a problem hiding this comment.
What about "Could not get a valid response from the server."?
There was a problem hiding this comment.
Nice, I'm good with that 👍 It's broad enough to include all kinds of errors that can prevent you from getting a response from the server.
| // Unfortunately the message might depend on the browser. | ||
| // If the browser reports being offline, we'll just assume that | ||
| // this is why the request failed. | ||
| if ( ! window.navigator.onLine ) { |
There was a problem hiding this comment.
Did you consider using is-network-error to recognize "informative errors" from generic ones and then throwing the informative one as the message field of the fetch_error?
I'm not even 100% sure how good an idea that would be, because the informative errors are i) rare; ii) very technical; iii) not localized.
There was a problem hiding this comment.
Did you consider using
is-network-error
Ah, I just said the same in the other comment thread. :) Well, implementation-wise, I don't know if I would actually require that package, or just inline that switch block, but that's a detail.
I'm not even 100% sure how good an idea that would be, because the informative errors are i) rare; ii) very technical; iii) not localized.
I don't think we need to actually present the non-connection errors to the user, as long as we already have those two classes of errors (network and non). The error should be logged in the console for anyone who can debug it.
There was a problem hiding this comment.
I'd prefer to use the is-network-error package explicitly, if we decide to do this kind of check. What I like about it that it supports also legacy messages for old versions of Chrome and Safari, something I'd never figure out myself. And the package is truly minimalistic.
| publish: __( 'Publishing failed because you were offline.' ), | ||
| private: __( 'Publishing failed because you were offline.' ), | ||
| future: __( 'Scheduling failed because you were offline.' ), | ||
| default: __( 'Updating failed because you were offline.' ), |
There was a problem hiding this comment.
I wonder if we can move "You were offline." to a separate string that we concat with the existing "publishing/updating/scheduling failed" string. That way translators will have less to translate. WDYT?
There was a problem hiding this comment.
Concatenating two separately translated strings isn't recommended, as far as I can remember. Based on the language, you may receive an odd complete translation.
There was a problem hiding this comment.
I agree in general if we were separating a single sentence, but perhaps it should be fine if they are 2 separate sentences?
There was a problem hiding this comment.
@swissspidy or @SergeyBiryukov might have a more definitive answer. I don't remember any exceptions to the rule, but they usually have a couple 😄
There was a problem hiding this comment.
Exactly, I intentionally inlined all possible sentences, the same way that we need to define all possible CPT labels one by one.
There was a problem hiding this comment.
Thanks. If there's a rule, sacrificing some extra work to add a few extra strings sounds like it's worth it. I'm just mindful about the translation efforts since every core string change requires hundreds of people's attention.
There was a problem hiding this comment.
If you wanna concatenate two sentences "Publishing failed." and "You are offline", that should be OK. But it's only 4 extra strings here which is also fine.
Alternatives would be to have a generic text only (e.g. "Action failed because you are probably offline"), or WP adding support for custom labels per post status.
Mamaduka
added
[Type] Enhancement
When handling a failed request, let api-fetch infer from the value of `navigator.onLine` whether the failure likely stems from the fact that the user is disconnected from the network. This still leaves out other kinds of network error, which right now will still trigger the fallback "Unexpected error" messaging. The spec for `Window.fetch` does not prescribe a standard error for network errors, meaning that browsers each implement their own messaging.
Props jsnajdr, tyxla
mcsf
force-pushed
the
fix/api-fetch-you-are-probably-offline
branch
from
383f438 to
aff32be
Compare
|
Size Change: +122 B (+0.01%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
I think this is looking great, and I'm happy with the new strings.
Some tests will need updating. Happy to have another look afterwards.
| throw { | ||
| code: 'offline_error', | ||
| message: __( | ||
| 'Unable to connect. Please check your Internet connection.' |
There was a problem hiding this comment.
I was unsure about the capitalization of "Internet" but I guess there's data behind it, as close to a tie as that might be 😅 .
There was a problem hiding this comment.
On the other hand, Core uses lower-case "internet". Although the sample is very small, there are just two messages that use the word:
- "Keeping your site updated is important for security. It also makes the internet a safer place for you and your readers."
- "The internet address of your network will be %s." (when configuring the network feature)
Capital I is used only when talking about Internet Explorer.
There was a problem hiding this comment.
I also checked the WP Glossary, and both are found throughout. :) Capitalised is how I personally write it, but I can go either way here. @jsnajdr, are you invested in the non-capitalised form?
There was a problem hiding this comment.
I'm not invested at all, was just curious about which convention is actually used in WordPress. 🙂 The Gutenberg repo also leans heavily towards lowercase, although it's all comments and docs, there's zero usage in code.
|
Flaky tests detected in b0c0b85. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17379162205
|
Good to go, @tyxla? |
What?
Part of #67141
Change the wording in the error notice that appears when saving/updating a post fails. Instead of "Updating failed. You are probably offline.", the notice might now read one of:
How?
When handling a failed request, let api-fetch infer from the value of
navigator.onLinewhether the failure likely stems from the fact that the user is disconnected from the network.This still leaves out other kinds of network error, which right now will still trigger the fallback "Unexpected error" messaging. The spec for
Window.fetchdoes not prescribe a standard error for network errors, meaning that browsers each implement their own messaging. See the parent issue for more technical context.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast