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

Jump to bottom

Interactivity API: Break up long hydration task in interactivity init #58227

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Interactivity API: Break up long hydration task in interactivity init #58227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
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
4 changes: 4 additions & 0 deletions packages/interactivity/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- Break up init with yielding to main to prevent long task from hydration. ([#58227](https://github.com/WordPress/gutenberg/pull/58227))

## 4.0.0 (2024-01-24)

### Enhancements
Expand Down
29 changes: 20 additions & 9 deletions packages/interactivity/src/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,26 @@ export const getRegionRootFragment = ( region ) => {
return regionRootFragments.get( region );
};

function yieldToMain() {
return new Promise( ( resolve ) => {
// TODO: Use scheduler.yield() when available.
luisherranz marked this conversation as resolved.
Show resolved Hide resolved
setTimeout( resolve, 0 );
} );
}

// Initialize the router with the initial DOM.
export const init = async () => {
document
.querySelectorAll( `[data-${ directivePrefix }-interactive]` )
.forEach( ( node ) => {
if ( ! hydratedIslands.has( node ) ) {
const fragment = getRegionRootFragment( node );
const vdom = toVdom( node );
hydrate( vdom, fragment );
}
} );
const nodes = document.querySelectorAll(
`[data-${ directivePrefix }-interactive]`
);

for ( const node of nodes ) {
if ( ! hydratedIslands.has( node ) ) {
await yieldToMain();
const fragment = getRegionRootFragment( node );
const vdom = toVdom( node );
await yieldToMain();
Comment on lines +38 to +42
Copy link
Member Author

@westonruter westonruter Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@luisherranz Actually, what is the purpose of hydratedIslands here? Could it be that the check for ! hydratedIslands.has( node ) could be done so that the condition is entered, but then yielding occurs so that it's possible that the island could be hydrated by another task? Could it be that toVdom() accidentally gets called on a hydrated island?

Copy link
Member

@luisherranz luisherranz Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

what is the purpose of hydratedIslands here?

Islands that are inside other islands belong to the same Preact application so they can share context and such. When an island is already hydrated as part of a parent, we skip its own hydration.

that it's possible that the island could be hydrated by another task?

I actually spotted a bug yesterday due to the fact that the router store was calling toVdom before the init. It's a rare edge case, but I'll fix it.

As toVdom is going to be a private, core-only API in WP 6.5, there should no be more problems. But in the future, if toVdom is exposed publicly, we can add a second argument to toVdom( node, isHydrating = false ) to know if it should add the nodes to the hydratedIslands map or not.

hydrate( vdom, fragment );
}
}
};
Loading