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.

Sign up for GitHub

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: Defer hydration until node is scrolled near the viewport #58284

Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from
Open

Interactivity API: Defer hydration until node is scrolled near the viewport #58284

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
645c945
Break long hydration task in interactivity init
westonruter Jan 25, 2024
6ac9d62
Yield to main between toVdom() and hydrate()
westonruter Jan 25, 2024
cffdb4d
Update changelog
westonruter Jan 25, 2024
4cc10a7
Delay hydration of nodes until they near the viewport
westonruter Jan 25, 2024
3770575
Yield to main after hydration
westonruter Jan 25, 2024
1f845c9
Check if isInputPending prior to hydration
westonruter Jan 25, 2024
d0587db
Merge remote-tracking branch 'origin/trunk' into try/interactivity-la…
westonruter Jan 26, 2024
cfe2b04
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into t…
westonruter Feb 6, 2024
d580cce
Merge branch 'trunk' into try/interactivity-lazy-hydration
westonruter Jul 23, 2024
fb15565
Remove now-discouraged use of isInputPending
westonruter Jul 23, 2024
3ec8de5
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into t…
westonruter Oct 10, 2024
6b95333
Merge branch 'trunk' into try/interactivity-lazy-hydration
westonruter Dec 19, 2024
5532ee4
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into t…
westonruter Dec 21, 2024
08720c4
Fix e2e tests by scrolling elements into viewport to trigger intersec…
sirreal Dec 26, 2024
d27985e
Always scroll to page bottom
sirreal Dec 26, 2024
88d679c
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into t…
westonruter Jan 7, 2025
abd3747
Use counter instead of Set of observed nodes
westonruter Jan 7, 2025
387ab6c
Restore missing initialVdom population
westonruter Jan 7, 2025
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
42 changes: 34 additions & 8 deletions packages/interactivity/src/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,38 @@ export const initialVdom = new WeakMap< Element, ComponentChild[] >();

// Initialize the router with the initial DOM.
export const init = async () => {
const pendingNodes = new Set();

const intersectionObserver = new window.IntersectionObserver(
async ( entries ) => {
for ( const entry of entries ) {
if ( ! entry.isIntersecting ) {
continue;
}

const node = entry.target;
intersectionObserver.unobserve( node );
pendingNodes.delete( node );
if ( pendingNodes.size === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the pendingNodes variable is only being populated to check this? I guess there's no way to check if the intersectionObserver is "empty"?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. There is no "observed count" exposed on the IntersectionObserver interface. We could change this to instead be a simple counter.

--- a/packages/interactivity/src/init.ts
+++ b/packages/interactivity/src/init.ts
@@ -29,7 +29,7 @@ export const initialVdom = new WeakMap< Element, ComponentChild[] >();
 
 // Initialize the router with the initial DOM.
 export const init = async () => {
-	const pendingNodes = new Set();
+	let observedNodeCount = 0;
 
 	const intersectionObserver = new window.IntersectionObserver(
 		async ( entries ) => {
@@ -40,8 +40,8 @@ export const init = async () => {
 
 				const node = entry.target;
 				intersectionObserver.unobserve( node );
-				pendingNodes.delete( node );
-				if ( pendingNodes.size === 0 ) {
+				observedNodeCount--;
+				if ( observedNodeCount === 0 ) {
 					intersectionObserver.disconnect();
 				}
 
@@ -76,8 +76,8 @@ export const init = async () => {
 		setTimeout( resolve, 0 );
 	} );
 
+	observedNodeCount = nodes.length;
 	for ( const node of nodes ) {
-		pendingNodes.add( node );
 		intersectionObserver.observe( node );
 	}
 };

This should have the same effect, with a slight benefit that the element references wouldn't be stored in the Set, meaning that there wouldn't be the possibility of a memory leak. (I should have used a WeakSet here originally, probably.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in abd3747

intersectionObserver.disconnect();
}

if ( ! hydratedIslands.has( node ) ) {
const fragment = getRegionRootFragment( node );
const vdom = toVdom( node );
Copy link
Member

Choose a reason for hiding this comment

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

In the old logic, this used to populate initialVdom. Is this no longer needed? If not, is there even any value in keeping the initialVdom const around?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems this is a merge conflict resolution on my part. This may be needed:

--- a/packages/interactivity/src/init.ts
+++ b/packages/interactivity/src/init.ts
@@ -48,6 +48,7 @@ export const init = async () => {
 				if ( ! hydratedIslands.has( node ) ) {
 					const fragment = getRegionRootFragment( node );
 					const vdom = toVdom( node );
+					initialVdom.set( node, vdom );
 					await splitTask();
 					hydrate( vdom, fragment );
 					await splitTask();

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it is only being used here:

gutenberg/packages/interactivity-router/src/index.ts

Lines 189 to 200 in ef7afef

// Initialize the router and cache the initial page using the initial vDOM.
// Once this code is tested and more mature, the head should be updated for
// region based navigation as well.
pages.set(
getPagePath( window.location.href ),
Promise.resolve(
regionsToVdom( document, {
vdom: initialVdom,
baseUrl: window.location.href,
} )
)
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #58496

Copy link
Member Author

Choose a reason for hiding this comment

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

Restored initialVdom.set( node, vdom ) in 387ab6c.

I found that this code in interactivity-router runs as the page loads when the "iAPI: full page client side navigation" experiment is enabled, and it also runs when interacting with a block that makes use of client-side navigation (i.e. the Query block when "Reload full page" is disabled). Nevertheless, the behavior of the page seems to work just as well whether or not initialVdom.set( node, vdom ) is added here.

@DAreRodz For this page cache, is it a problem that initialVdom is not initially populated with all of the interactive regions of the page since they get added only when they come into view?

await splitTask();
hydrate( vdom, fragment );
await splitTask();
Copy link
Member

Choose a reason for hiding this comment

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

Why does this yield at the end of the process? In the original code that happened before, right above the getRegionRootFragment call.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I recall, I moved it here because given that this is now inside of an IntersectionObserver callback, it is already running as part of a separate task. Therefore, it doesn't make sense to splitTask at the beginning of the loop since then essentially there would be two yieldings to the main thread back to back. So this is why I moved the await splitTask() calls further down here.

}
}
},
{
root: null, // To watch for intersection relative to the device's viewport.
rootMargin: '100% 0% 100% 0%', // Intersect when within 1 viewport approaching from top or bottom.
threshold: 0.0, // As soon as even one pixel is visible.
}
);

const nodes = document.querySelectorAll(
`[data-${ directivePrefix }-interactive]`
);
Expand All @@ -45,13 +77,7 @@ export const init = async () => {
} );

for ( const node of nodes ) {
if ( ! hydratedIslands.has( node ) ) {
await splitTask();
const fragment = getRegionRootFragment( node );
const vdom = toVdom( node );
initialVdom.set( node, vdom );
await splitTask();
hydrate( vdom, fragment );
}
pendingNodes.add( node );
intersectionObserver.observe( node );
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be a good idea to keep the hydratedIslands.has check here? No need to even observe nodes that are already hydrated, e.g. if init was called multiple times. Unlikely, but probably a good safety net to have.

Suggested change
pendingNodes.add( node );
intersectionObserver.observe( node );
if ( ! hydratedIslands.has( node ) ) {
pendingNodes.add( node );
intersectionObserver.observe( node );
}

Copy link
Member Author

@westonruter westonruter Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The hydratedIslands.has() check was moved to the IntersectionObserver callback. It could be added here as well, but after abd3747 that would complicate things a bit since it could be that the nodes.length number would end up being larger than the number of nodes actually observed, meaning the IntersectionObserver would never get disconnected.

}
};
12 changes: 11 additions & 1 deletion test/e2e/specs/interactivity/directive-each.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ test.describe( 'data-wp-each', () => {

test.beforeEach( async ( { interactivityUtils: utils, page } ) => {
await page.goto( utils.getLink( 'test/directive-each' ) );

// Scroll to page bottom to trigger hydration of out-of-viewport interactive regions.
await page.evaluate(
`window.scrollTo( {
top: document.body.scrollHeight,
left: 0,
behavior: 'instant',
} );`
);
} );

test.afterAll( async ( { interactivityUtils: utils } ) => {
Expand Down Expand Up @@ -509,7 +518,8 @@ test.describe( 'data-wp-each', () => {
test( `does not error with non-iterable values: ${ testId }`, async ( {
page,
} ) => {
await expect( page.getByTestId( testId ) ).toBeEmpty();
const element = page.getByTestId( testId );
await expect( element ).toBeEmpty();
} );
}

Expand Down
Loading