Interactivity API: Add timeout option to navigate()
#54474
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DAreRodz
added
[Type] Enhancement
DAreRodz
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad,
ajitbohra and
luisherranz
as code owners
24 tasks
|
Size Change: +91.2 kB (+6%) 🔍 Total Size: 1.62 MB
ℹ️ View Unchanged
|
SantosGuillamot
requested changes
Sep 15, 2023
There was a problem hiding this comment.
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. |
21 tasks
|
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 🙂 |
luisherranz
approved these changes
Sep 19, 2023
SantosGuillamot
approved these changes
Sep 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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