Skip to content
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: Override unfinished navigate calls #54201

Merged
merged 6 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"apiVersion": 2,
"name": "test/router-navigate",
"title": "E2E Interactivity tests - router navigate",
"category": "text",
"icon": "heart",
"description": "",
"supports": {
"interactivity": true
},
"textdomain": "e2e-interactivity",
"viewScript": "router-navigate-view",
"render": "file:./render.php"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php
/**
* HTML for testing the router navigate function.
*
* @package gutenberg-test-interactive-blocks
* @phpcs:disable VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable
*/

?>


<div data-wp-interactive data-wp-navigation-id="region-1">
<h2 data-testid="title"><?php echo $attributes['title']; ?></h2>

<output
data-testid="router navigations"
data-wp-text="state.router.navigations"
>NaN</output>
<output
data-testid="router status"
data-wp-text="state.router.status"
>undefined</output>

<?php
if ( isset( $attributes['links'] ) ) {
foreach ( $attributes['links'] as $key => $link ) {
$i = $key += 1;
echo <<<HTML
<a
data-testid="link $i"
data-wp-on--click="actions.router.navigate"
href="$link"
>link $i</a>
HTML;
}
}
?>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
( ( { wp } ) => {
/**
* WordPress dependencies
*/
const { store, navigate } = wp.interactivity;

store( {
state: {
router: {
status: 'idle',
navigations: 0,
}
},
actions: {
router: {
navigate: async ( { state, event: e } ) => {
e.preventDefault();

state.router.navigations += 1;
state.router.status = 'busy';

await navigate( e.target.href );

state.router.navigations -= 1;

if ( state.router.navigations === 0) {
state.router.status = 'idle';
}
},
},
},
} );
} )( window );
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- Improve `navigate()` to render only the result of the last call when multiple happen simultaneously. ([#54201](https://github.com/WordPress/gutenberg/pull/54201))

## 2.2.0 (2023-08-31)

### Enhancements
Expand Down
10 changes: 10 additions & 0 deletions packages/interactivity/src/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,21 @@ const renderRegions = ( page ) => {
} );
};

// Variable to store the current navigation.
let navigatingTo = '';

// Navigate to a new page.
export const navigate = async ( href, options = {} ) => {
const url = cleanUrl( href );
navigatingTo = url;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the logic for using url instead of href here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. 😄 I think I assumed that, as the url would be the same for href with hashes, both navigations would wait for the same Promise so that they would happen in order and there would be no problem.

I missed considering that:

  1. You could use force and request the HTML again.
  2. Both navigations would trigger a render, which is bad.

I added a failing test in 83ae05f and fixed it in 578029c.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it's a bit safer this way. That way, the user will end up with the correct hash in the browser URL (even though we don't support scrolling to the id yet).

Thanks David!

prefetch( url, options );
const page = await pages.get( url );

// 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.
if ( navigatingTo !== url ) return;

if ( page ) {
renderRegions( page );
window.history[ options.replace ? 'replaceState' : 'pushState' ](
Expand Down
77 changes: 77 additions & 0 deletions test/e2e/specs/interactivity/router-navigate.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* Internal dependencies
*/
import { test, expect } from './fixtures';

test.describe( 'Router navigate', () => {
test.beforeAll( async ( { interactivityUtils: utils } ) => {
await utils.activatePlugins();
const link2 = await utils.addPostWithBlock( 'test/router-navigate', {
alias: 'router navigate - link 2',
attributes: { title: 'Link 2' },
} );
const link1 = await utils.addPostWithBlock( 'test/router-navigate', {
alias: 'router navigate - link 1',
attributes: { title: 'Link 1' },
} );
await utils.addPostWithBlock( 'test/router-navigate', {
alias: 'router navigate - main',
attributes: { title: 'Main', links: [ link1, link2 ] },
} );
} );

test.beforeEach( async ( { interactivityUtils: utils, page } ) => {
await page.goto( utils.getLink( 'router navigate - main' ) );
} );

test.afterAll( async ( { interactivityUtils: utils } ) => {
await utils.deactivatePlugins();
await utils.deleteAllPosts();
} );

test( 'should update the HTML only for the latest navigation', async ( {
page,
interactivityUtils: utils,
} ) => {
const link1 = utils.getLink( 'router navigate - link 1' );
const link2 = utils.getLink( 'router navigate - link 2' );

const navigations = page.getByTestId( 'router navigations' );
const status = page.getByTestId( 'router status' );
const title = page.getByTestId( 'title' );

await expect( navigations ).toHaveText( '0' );
await expect( status ).toHaveText( 'idle' );

let resolveLink1: Function;
let resolveLink2: Function;

await page.route( link1, async ( route ) => {
await new Promise( ( r ) => ( resolveLink1 = r ) );
await route.continue();
} );
await page.route( link2, async ( route ) => {
await new Promise( ( r ) => ( resolveLink2 = r ) );
await route.continue();
} );

await page.getByTestId( 'link 1' ).click();
await page.getByTestId( 'link 2' ).click();

await expect( navigations ).toHaveText( '2' );
await expect( status ).toHaveText( 'busy' );
await expect( title ).toHaveText( 'Main' );

await Promise.resolve().then( () => resolveLink2() );

await expect( navigations ).toHaveText( '1' );
await expect( status ).toHaveText( 'busy' );
await expect( title ).toHaveText( 'Link 2' );

await Promise.resolve().then( () => resolveLink1() );

await expect( navigations ).toHaveText( '0' );
await expect( status ).toHaveText( 'idle' );
await expect( title ).toHaveText( 'Link 2' );
} );
} );