-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Interactivity API: Add timeout
option to navigate()
#54474
Conversation
Size Change: +91.2 kB (+6%) 🔍 Total Size: 1.62 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.
I have a couple of comments:
- We are focusing the first element even if the timeout is reached.
- The loading bar disappears after the timeout, even if it hasn't navigated yet to the new page.
These aspects combined give me the impression that it has navigated even if we are still in the same page. Could we not focus the first element and keep the loading bar until it actually loads the next page?
I made a quick video explaining what I mean:
Query.navigation.-.15.September.2023.mp4
Thanks, @SantosGuillamot; that's good feedback. 👌 The We could maybe return a boolean from the |
Maybe navigate should never resolve if there was a timeout. |
I made |
Flaky tests detected in 58cae72. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6234144709
|
Why is throwing an error better than not resolving the function? |
I don't think it's better―for me, it was just a matter of convenience. 😅 Actually, the In addition, it simplifies the code as we don't have to write something to handle when the timeout finishes and then await a never-resolving If you prefer, here you have a code that makes diff --git a/packages/interactivity/src/router.js b/packages/interactivity/src/router.js
index e4f53970fb..120205254f 100644
--- a/packages/interactivity/src/router.js
+++ b/packages/interactivity/src/router.js
@@ -92,15 +92,20 @@ export const navigate = async ( href, options = {} ) => {
prefetch( url, options );
const pagePromise = pages.get( url );
+
+ let hasTimedOut = false;
const timeoutPromise = new Promise( ( r ) => {
timeoutId = setTimeout( r, options.timeout ?? 10000 );
} ).then( () => {
window.location.assign( href );
- throw new Error( 'timeout' );
+ hasTimedOut = true;
} );
const page = await Promise.race( [ pagePromise, timeoutPromise ] );
+ // Never ever resolve.
+ if ( ! page && hasTimedOut ) await new Promise( () => {} );
+
// Once the page is fetched, the destination URL could have changed (e.g.,
// by clicking another link in the meantime). If so, bail out, and let the
// newer execution to update the HTML.
|
This reverts commit 92f1333.
Let's go with the unresolved then to avoid throwing errors on people's websites 🙂 |
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.
Looks good to me. Thanks, David 🙂
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.
It seems to be working fine now 🙂
What?
This PR adds an option to the
navigate()
function exposed by the Interactivity API.When the timeout expires, the browser reloads the page you are visiting instead of waiting.
Part of #54291
Why?
To add a default behavior when a navigation takes too long.
How?
Inside
navigation()
, where the function awaits the request to be resolved, I added another promise that fulfills when the timeout passes. Then, I usedPromise.race()
to return the resolved value of the fastest promise.Testing Instructions
You can use the Enhanced pagination feature of the Query Loop block to test this.
Screenshots or screencast
Screen.Recording.2023-09-14.at.21.12.35.mov