Interactivity API: Delay block hydration to allow interactive block stores to initialize

[westonruter]

Fixes #66691.

When scheduler.yield() is available, the splitTask() function is implemented to use it. When a task is split with scheduler.yield() it results in the tail being added to the front of the task queue. In contrast, when splitTask() is implemented with setTimeout() (when scheduler.yield() is not available), then the tail of the split task is added to the end of the task queue. It appears there is a race condition between the call to init.

In particular, given this init function:

gutenberg/packages/interactivity/src/init.ts

Lines 30 to 46 in 7f49b39

	// Initialize the router with the initial DOM.
	export const init = async () => {
	const nodes = document.querySelectorAll(
	`[data-${ directivePrefix }-interactive]`
	);
	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 );
	}
	}
	};

It is getting invoked before the code in a block's view.js is invoked, for example:

gutenberg/packages/block-library/src/image/view.js

Line 22 in 7f49b39

const { state, actions, callbacks } = store(

When splitTask() is was using setTimeout() then the calls to await splitTask() had the effect of delaying the hydration until after the blocks' view modules execute. But when splitTask() is implemented with scheduler.yield() the hydration logic is executing before the interactive blocks' view module (i.e. the store initialization) executes.

Of note, the error also occurs if await splitTask() is eliminated from init function as well, indicating that it was not correctly accounting for delaying the Interactivity API's init until after the interactive blocks have all initialized. There seems to be a dependency problem here, where hydration of an interactive block should be blocked until its corresponding view script module has loaded. This seems to be related to:

• Block and Component Lazy Loading #2768
• Support for lazy hydration techniques: idle and view #52723
• Blocks: Introduce a way to enqueue view scripts only when needed for interactivity

In the mean time, adding a setTimeout() to handle the delay seems to do the trick.

Code execution with splitTask implemented with setTimeout

gutenberg/packages/interactivity/src/init.ts

Lines 31 to 38 in 7f49b39

	export const init = async () => {
	const nodes = document.querySelectorAll(
	`[data-${ directivePrefix }-interactive]`
	);
	for ( const node of nodes ) {
	if ( ! hydratedIslands.has( node ) ) {
	await splitTask();

gutenberg/packages/block-library/src/image/view.js

Line 22 in 7f49b39

const { state, actions, callbacks } = store(

gutenberg/packages/interactivity/src/init.ts

Lines 39 to 46 in 7f49b39

	const fragment = getRegionRootFragment( node );
	const vdom = toVdom( node );
	initialVdom.set( node, vdom );
	await splitTask();
	hydrate( vdom, fragment );
	}
	}
	};

Code execution with splitTask implemented with scheduler.yield

gutenberg/packages/interactivity/src/init.ts

Lines 30 to 46 in 7f49b39

	// Initialize the router with the initial DOM.
	export const init = async () => {
	const nodes = document.querySelectorAll(
	`[data-${ directivePrefix }-interactive]`
	);
	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 );
	}
	}
	};

gutenberg/packages/block-library/src/image/view.js

Line 22 in 7f49b39

const { state, actions, callbacks } = store(

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: cbravobernal <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: annezazu <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[michalczaplinski]

Thanks for investigating and creating the PR @westonruter ! 🙂

The current solution does fix the problem. However, even with the current workaround, the issue will persist if I remove or comment out the splitTask () calls in init(). I think it's not optimal that we have an implicit dependency on splitTask() which should just be a performance optimization.

I'm currently trying to figure out a better way to handle this.

[cbravobernal]

As @michalczaplinski commented, we can work in a follow-up PR to find a better solution. Meanwhile, I'll merge it and include it on today's Gutenberg release.

[cbravobernal]

I just cherry-picked this PR to the release/19.6 branch to get it included in the next release: f4b013f

[westonruter]

The current solution does fix the problem. However, even with the current workaround, the issue will persist if I remove or comment out the splitTask () calls in init(). I think it's not optimal that we have an implicit dependency on splitTask() which should just be a performance optimization.

@michalczaplinski Really? I commented out the splitTask() calls and I was getting the error still. I just checked again, and I continue to get the errors. With this merged PR, I commented out these lines:

diff --git a/packages/interactivity/src/init.ts b/packages/interactivity/src/init.ts
index fa1eec51c3e..adc17a6458e 100644
--- a/packages/interactivity/src/init.ts
+++ b/packages/interactivity/src/init.ts
@@ -40,17 +40,17 @@ export const init = async () => {
 	 * This occurs when splitTask() is implemented with scheduler.yield() as opposed to setTimeout(), as with the former
 	 * split tasks are added to the front of the task queue whereas with the latter they are added to the end of the queue.
 	 */
-	await new Promise( ( resolve ) => {
-		setTimeout( resolve, 0 );
-	} );
+	// await new Promise( ( resolve ) => {
+	// 	setTimeout( resolve, 0 );
+	// } );
 
 	for ( const node of nodes ) {
 		if ( ! hydratedIslands.has( node ) ) {
-			await splitTask();
+			// await splitTask();
 			const fragment = getRegionRootFragment( node );
 			const vdom = toVdom( node );
 			initialVdom.set( node, vdom );
-			await splitTask();
+			// await splitTask();
 			hydrate( vdom, fragment );
 		}
 	}

And I get these errors:

[michalczaplinski]

@westonruter, I'm unsure what you mean 🙂. I think that we're in agreement that the Cannot access 'state'... issue persists even if you comment out the splitTask() calls in init(). This is what I meant with my previous comment.

[westonruter]

@michalczaplinski Oh, hah, sorry! I misunderstood.

[michalczaplinski]

Sorry, I should have been more clear earlier too 🙂 . Here's my expanded explanation with additional details:

Adding await new Promise(...) inside the init() (which was done in the current PR) fixes #66691. This fix ALSO works even if you comment out the splitTask() calls.

However, relying on await new Promise(...) inside the init() can be brittle. Imagine a custom block adding another await new Promise(...) at the module level in its view.js file. This will cause the same problem to reappear because now the individual view.js file adds a task to the end of the task queue.

I'm not sure if there's a better solution at this point than what was merged in the current PR. @DAreRodz and I have considered wrapping the init() with DOMContentloaded as it was removed here, but that does not solve the problem either.

[westonruter]

I'm not sure if there's a better solution at this point than what was merged in the current PR. @DAreRodz and I have considered wrapping the init() with DOMContentloaded as it was removed here, but that does not solve the problem either.

AFAIK, the DOMContentLoaded event won't work specifically because these are script modules, and script modules execute after this event. So if any script module tries to add a DOMContentLoaded event, the event will never fire.

[michalczaplinski]

AFAIK, the DOMContentLoaded event won't work specifically because these are script modules, and script modules execute after this event. So if any script module tries to add a DOMContentLoaded event, the event will never fire.

right, exactly 🙂

[luisherranz]

@DAreRodz let's take a look at this for WordPress 6.8 to see if we can ensure there are no issues when stores initialize after the init function is called, which is necessary for future implementations, like lazy loading blocks.

That being said, it's okay for the blocks that load initially to do so before the init function, as it is happening today (because we can optimize a few things in the init function itself).