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: Improvements to the experimental full-page navigation #64067

Merged
merged 28 commits into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b34f5b6
fix: Add initial data population in interactivity router
michalczaplinski Jul 29, 2024
894bc9a
chore: Update element.innerText to element.textContent in head.ts file
michalczaplinski Aug 1, 2024
d132d26
feat: Exclude src and href attributes when copying head element attri…
michalczaplinski Aug 1, 2024
2f8c93b
chore: Populate initial data in interactivity router
michalczaplinski Aug 1, 2024
d8500ec
chore: Move Populate initial data in interactivity router
michalczaplinski Aug 1, 2024
804b7c1
Wait for `load` event of script element before returning from `fetchH…
michalczaplinski Aug 1, 2024
0f2a51b
feat: Update head tags to improve prefetching of scripts and stylesheets
michalczaplinski Aug 21, 2024
eac0deb
Do not load interactivity script modules in development mode when ful…
michalczaplinski Aug 28, 2024
af136bc
Format interactivity-api.php
michalczaplinski Aug 28, 2024
424da1c
Update interactivity script module registration to use version from a…
michalczaplinski Sep 11, 2024
56a9b08
empty commit
michalczaplinski Sep 12, 2024
bc0c933
Rename populateInitialData to populateServerData
michalczaplinski Sep 12, 2024
78d2d88
remove populateServerData
michalczaplinski Sep 12, 2024
a5a40cc
try: remove the webpack comment
michalczaplinski Sep 12, 2024
3cef459
try: remove the await import()
michalczaplinski Sep 12, 2024
2811a8c
bring back the async import
michalczaplinski Sep 12, 2024
79bd1c9
Move headElements to head.ts
michalczaplinski Sep 12, 2024
06fd98d
Revert "try: remove the webpack comment"
michalczaplinski Sep 17, 2024
9983180
default_version => router_version
michalczaplinski Sep 17, 2024
0186d2a
Remove the changes to interactivity-api.php
michalczaplinski Sep 24, 2024
3e49d89
Merge branch 'trunk' into fix/full-page-navigation-server-state
michalczaplinski Sep 24, 2024
a9469d0
Make `renderRegions` async
DAreRodz Sep 24, 2024
13c881a
Update TS type of the stylesheets variable
michalczaplinski Oct 2, 2024
0b550f9
Replace Array.from() with direct forEach() on NodeList in head.ts:
michalczaplinski Oct 2, 2024
cf73a66
Check if href is null
michalczaplinski Oct 2, 2024
375d4ee
Clarify why we only prefetch script modules
michalczaplinski Oct 3, 2024
a539b9a
Add changelog
michalczaplinski Oct 8, 2024
ba856cf
Merge branch 'trunk' into fix/full-page-navigation-server-state
michalczaplinski Oct 8, 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
8 changes: 8 additions & 0 deletions packages/interactivity-router/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

## Unreleased

### Enhancements

- Improvements to the experimental full-page navigation ([#64067](https://github.com/WordPress/gutenberg/pull/64067)):
- Remove the `src` attributes from prefetched script tags.
- Use `.textContent` instead of `.innerText` to set `<script>` contents.
- Use `populateInitialData()` with state from the server.
- Wait for the `load` event of the script element before evaluating it.

## 2.9.0 (2024-10-03)

## 2.8.0 (2024-09-19)
Expand Down
114 changes: 68 additions & 46 deletions packages/interactivity-router/src/head.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
/**
* The cache of prefetched stylesheets and scripts.
*/
export const headElements = new Map<
string,
{ tag: HTMLElement; text?: string }
>();

/**
* Helper to update only the necessary tags in the head.
*
Expand Down Expand Up @@ -29,6 +37,14 @@ export const updateHead = async ( newHead: HTMLHeadElement[] ) => {
}
}

await Promise.all(
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
[ ...headElements.entries() ]
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
.filter( ( [ , { tag } ] ) => tag.nodeName === 'SCRIPT' )
.map( async ( [ url ] ) => {
await import( /* webpackIgnore: true */ url );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the webpack comment here. Otherwise, webpack will try to code-split here into a new chunk and it does not permit using fully dynamic paths.

} )
);

// Prepare new assets.
const toAppend = [ ...newHeadMap.values() ];

Expand All @@ -41,60 +57,66 @@ export const updateHead = async ( newHead: HTMLHeadElement[] ) => {
* Fetches and processes head assets (stylesheets and scripts) from a specified document.
*
* @async
* @param doc The document from which to fetch head assets. It should support standard DOM querying methods.
* @param headElements A map of head elements to modify tracking the URLs of already processed assets to avoid duplicates.
* @param headElements.tag
* @param headElements.text
* @param doc The document from which to fetch head assets. It should support standard DOM querying methods.
*
* @return Returns an array of HTML elements representing the head assets.
*/
export const fetchHeadAssets = async (
doc: Document,
headElements: Map< string, { tag: HTMLElement; text: string } >
doc: Document
): Promise< HTMLElement[] > => {
const headTags = [];
const assets = [
{
tagName: 'style',
selector: 'link[rel=stylesheet]',
attribute: 'href',
},
{ tagName: 'script', selector: 'script[src]', attribute: 'src' },
];
for ( const asset of assets ) {
const { tagName, selector, attribute } = asset;
const tags = doc.querySelectorAll<
HTMLScriptElement | HTMLStyleElement
>( selector );

// Use Promise.all to wait for fetch to complete
await Promise.all(
Array.from( tags ).map( async ( tag ) => {
const attributeValue = tag.getAttribute( attribute );
if ( ! headElements.has( attributeValue ) ) {
try {
const response = await fetch( attributeValue );
const text = await response.text();
headElements.set( attributeValue, {
tag,
text,
} );
} catch ( e ) {
// eslint-disable-next-line no-console
console.error( e );
}
}

const headElement = headElements.get( attributeValue );
const element = doc.createElement( tagName );
element.innerText = headElement.text;
for ( const attr of headElement.tag.attributes ) {
element.setAttribute( attr.name, attr.value );
// We only want to fetch module scripts because regular scripts (without
// `async` or `defer` attributes) can depend on the execution of other scripts.
// Scripts found in the head are blocking and must be executed in order.
const scripts = doc.querySelectorAll< HTMLScriptElement >(
'script[type="module"][src]'
);

scripts.forEach( ( script ) => {
const src = script.getAttribute( 'src' );
if ( ! headElements.has( src ) ) {
// add the <link> elements to prefetch the module scripts
const link = doc.createElement( 'link' );
link.rel = 'modulepreload';
link.href = src;
document.head.append( link );
headElements.set( src, { tag: script } );
}
} );

const stylesheets = doc.querySelectorAll< HTMLLinkElement >(
'link[rel=stylesheet]'
);

await Promise.all(
Array.from( stylesheets ).map( async ( tag ) => {
Comment on lines +92 to +93
Copy link
Member

@sirreal sirreal Oct 2, 2024

Choose a reason for hiding this comment

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

The possible fetch failure is one of the more fragile places here and it's handled.

However, anything else that fails in these promises will cause the entire function to reject. I don't think that's desired (based on the try/catch on fetch). allSettled may be better to use here, since we're not interested in the result and want to make a best effort here, then the function can continue whether or not all the promises resolve.

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 idea indeed, but could we do it in a follow-up PR?

I didn't intend to change this behavior in this PR.

const href = tag.getAttribute( 'href' );
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
if ( ! href ) {
return;
}

if ( ! headElements.has( href ) ) {
try {
const response = await fetch( href );
const text = await response.text();
Comment on lines +101 to +102
Copy link
Member

Choose a reason for hiding this comment

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

This should check for response.ok and it would probably be good to check the content type response as well.

if ( ! response.ok ) { return; }
if ( 'text/css' !== response.headers.get('Content-Type') ) { return; }

These could throw an error to enter the catch and be logged to the console.

Copy link
Member

Choose a reason for hiding this comment

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

I'm rather new to this and lack context. Is there a reason to inject link tags instead of loading the style elements inline?

One reason I'm curious about this is that the browser will decide how many resources on the page to fetch in parallel, but I believe these fetch calls fetch all the resources in parallel without any limits. Ideally it seems like link tags would be used but I assume that was explored and discarded as an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should check for response.ok and it would probably be good to check the content type response as well.

That totally makes sense, but could we also leave this for a follow-up PR? I didn't intend to change this behavior in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm rather new to this and lack context. Is there a reason to inject link tags instead of loading the style elements inline?

I'm not sure I follow here, sorry. We are injecting <link> tags to prefetch modules.

Or did you mean if there's a reason NOT to inject link tags for CSS assets?

Copy link
Member

Choose a reason for hiding this comment

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

This: why not use the same link tags for CSS assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can do that! (#60951 (comment))

I just wasn't planning on doing that in this iteration 🙂

headElements.set( href, {
tag,
text,
} );
} catch ( e ) {
// eslint-disable-next-line no-console
console.error( e );
}
headTags.push( element );
} )
);
}
}

const headElement = headElements.get( href );
const styleElement = doc.createElement( 'style' );
styleElement.textContent = headElement.text;
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved

headTags.push( styleElement );
} )
);

return [
doc.querySelector( 'title' ),
Expand Down
56 changes: 30 additions & 26 deletions packages/interactivity-router/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { store, privateApis, getConfig } from '@wordpress/interactivity';
/**
* Internal dependencies
*/
import { fetchHeadAssets, updateHead } from './head';
import { fetchHeadAssets, updateHead, headElements } from './head';

const {
directivePrefix,
Expand Down Expand Up @@ -54,7 +54,6 @@ const navigationMode: 'regionBased' | 'fullPage' =

// The cache of visited and prefetched pages, stylesheets and scripts.
const pages = new Map< string, Promise< Page | false > >();
const headElements = new Map< string, { tag: HTMLElement; text: string } >();

// Helper to remove domain and hash from the URL. We are only interesting in
// caching the path and the query.
Expand Down Expand Up @@ -87,7 +86,7 @@ const regionsToVdom: RegionsToVdom = async ( dom, { vdom } = {} ) => {
let head: HTMLElement[];
if ( globalThis.IS_GUTENBERG_PLUGIN ) {
if ( navigationMode === 'fullPage' ) {
head = await fetchHeadAssets( dom, headElements );
head = await fetchHeadAssets( dom );
regions.body = vdom
? vdom.get( document.body )
: toVdom( dom.body );
Expand All @@ -108,31 +107,34 @@ const regionsToVdom: RegionsToVdom = async ( dom, { vdom } = {} ) => {
};

// Render all interactive regions contained in the given page.
const renderRegions = ( page: Page ) => {
batch( () => {
if ( globalThis.IS_GUTENBERG_PLUGIN ) {
if ( navigationMode === 'fullPage' ) {
// Once this code is tested and more mature, the head should be updated for region based navigation as well.
updateHead( page.head );
const fragment = getRegionRootFragment( document.body );
const renderRegions = async ( page: Page ) => {
if ( globalThis.IS_GUTENBERG_PLUGIN ) {
if ( navigationMode === 'fullPage' ) {
// Once this code is tested and more mature, the head should be updated for region based navigation as well.
await updateHead( page.head );
const fragment = getRegionRootFragment( document.body );
batch( () => {
populateServerData( page.initialData );
render( page.regions.body, fragment );
}
} );
}
if ( navigationMode === 'regionBased' ) {
}
if ( navigationMode === 'regionBased' ) {
const attrName = `data-${ directivePrefix }-router-region`;
batch( () => {
populateServerData( page.initialData );
const attrName = `data-${ directivePrefix }-router-region`;
document
.querySelectorAll( `[${ attrName }]` )
.forEach( ( region ) => {
const id = region.getAttribute( attrName );
const fragment = getRegionRootFragment( region );
render( page.regions[ id ], fragment );
} );
}
if ( page.title ) {
document.title = page.title;
}
} );
} );
}
if ( page.title ) {
document.title = page.title;
}
};

/**
Expand All @@ -156,7 +158,7 @@ window.addEventListener( 'popstate', async () => {
const pagePath = getPagePath( window.location.href ); // Remove hash.
const page = pages.has( pagePath ) && ( await pages.get( pagePath ) );
if ( page ) {
renderRegions( page );
await renderRegions( page );
// Update the URL in the state.
state.url = window.location.href;
} else {
Expand All @@ -170,13 +172,15 @@ window.addEventListener( 'popstate', async () => {
if ( globalThis.IS_GUTENBERG_PLUGIN ) {
if ( navigationMode === 'fullPage' ) {
// Cache the scripts. Has to be called before fetching the assets.
[].map.call( document.querySelectorAll( 'script[src]' ), ( script ) => {
headElements.set( script.getAttribute( 'src' ), {
tag: script,
text: script.textContent,
} );
} );
await fetchHeadAssets( document, headElements );
[].map.call(
document.querySelectorAll( 'script[type="module"][src]' ),
michalczaplinski marked this conversation as resolved.
Show resolved Hide resolved
( script ) => {
headElements.set( script.getAttribute( 'src' ), {
tag: script,
} );
}
);
await fetchHeadAssets( document );
}
}
pages.set(
Expand Down
Loading