iAPI Router: Handle styles assets on region-based navigation #67826
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 tasks
|
Size Change: -494 B (-0.03%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Gave it an initial review and overall the idea of using the adoptedStyleSheets is looking good to me. 👍
I have one concern though: In this PR, we're effectively changing the behavior of prefetching. Previously, to "prefetch" meant to fetch the HTML as well as the JS and CSS assets of a page.
Now, with the changes introduced here, prefetching only fetches the HTML of a new page and fetching of JS and CSS assets is delayed until a navigation (actions.navigate()) happens.
This might be what we want but it should be a conscious decision.
Co-authored-by: Michal <mmczaplinski@gmail.com>
Oh, I see what you mean. Before, the In practice, it would behave the same as before, as the gutenberg/packages/interactivity-router/src/index.ts Lines 330 to 334 in 769342b I have to admit this was an unconscious change. 😄 But maybe it is good to keep it this way? 🤔 |
Yup, exactly. Your approach is also more straightforward to implement as you can see by the number of lines you deleted 😄
Maybe? 🙂 Your approach is also the one taken by Hotwired. They only prefetch the HTML and then merge the head of previous & new pages. Let's first test this approach on a "real" site to see if the performance is acceptable. Previously, the new page would load instantly since everything (HTML, CSS & JS) was already loaded. Now, there might be a slight delay if we have to wait for the CSS & JS. If the performance is not acceptable, we add the prefetching similar to how it worked before. It worked fine for JS modules using |
|
Flaky tests detected in a423826. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12396914924
|
The script modules logic is going to change to support dynamic import maps, so we could address this in a subsequent PR. Don't worry. 😄
That's how it works in this PR, right? 🤔 The moment gutenberg/packages/interactivity-router/src/index.ts Lines 130 to 154 in 769342b |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Hi 👋. Just dropping in to say I'm watching this and subsequent module/js loading with much excitement. Right now I'm getting around this in more complex IAPI routing applications by enqueueing on page load the modules/scripts and styles I know I'll need for new blocks that can possibly render on a new router navigation, it works, but having this be dynamic and automatic would be very welcome. |
luisherranz
approved these changes
Dec 19, 2024
There was a problem hiding this comment.
This looks fantastic. Thanks, David! Congratulations on the tests. Also, very good.
I just have one question, but it's not limiting. Feel free to merge it so people can try it as soon as possible.
Regarding preloading of CSS and JS assets, we can discuss it and add it back in a subsequent PR.
Comment on lines
+41
to
+49
| if ( elementSheet ) { | ||
| return getCachedSheet( sheetId, () => { | ||
| const sheet = new CSSStyleSheet(); | ||
| for ( const { cssText } of elementSheet.cssRules ) { | ||
| sheet.insertRule( withAbsoluteUrls( cssText, sheetUrl ) ); | ||
| } | ||
| return Promise.resolve( sheet ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
In which case the element.sheet (CSSStyleSheet instance) exists but the URLs are wrong and need to be corrected? Is element.sheet not only present when the link tag is actually on DOM, therefore only on the initial page load?
Not worried :) A follow-up PR sounds good 👍
Yes, that's right. I didn't mean to imply otherwise 🙂. I also reviewed the logic for In general, some other things can be present in
|
DAreRodz
added a commit
to woocommerce/gutenberg
that referenced
this pull request
Feb 18, 2025
…WordPress#67826)" This reverts commit 0a5be06.
Kallyan01
pushed a commit
to Kallyan01/gutenberg
that referenced
this pull request
Feb 24, 2025
…WordPress#69222) * Revert "iAPI Router: Fix CSS rule order in some constructed style sheets (WordPress#68923)" This reverts commit 4aad5e3. * Revert "iAPI Router: Handle styles assets on region-based navigation (WordPress#67826)" This reverts commit 12f7764. * Add changelog entry
DAreRodz
added a commit
that referenced
this pull request
Jun 18, 2025
…igation (#70353) * Revert "Remove the iAPI full-page experiment entirely (#70228)" This reverts commit f693991. * Reapply "iAPI Router: Handle styles assets on region-based navigation (#67826)" This reverts commit 0a5be06. * Reapply "iAPI Router: Fix CSS rule order in some constructed style sheets (#68923)" This reverts commit a53bb20. * Clone style elements and enable/disable them * Combine `media` and `disabled` * Add prefetch callback to style tests * Remove `async` from `applyStyles` * Add basic error handling * Rename `baseUrl` to `url` * Improve comment wrappers around new styles sheets * Minor fixes * Use a SCS algorithm to add new style sheets * Refactor and document `updateStylesWithSCS` * Fix promises returned by `updateStylesWithSCS` * First unit tests (claude) * Test improvements (claude) * Tests for shortestCommonSupersequence (claude) * Move scs tests a folder up * Refactor scs tests a bit * Refactor `updateStylesWithSCS` and `applyStyles` unit tests * Add new test for media attributes * Fix last appended elements * Rename and document `areStylesEqual` * Improve `areStyleEqual` TSDoc * Add more TSDocs * Add TSDoc for `shortestCommonSupersequence` * Test stylesheet loading fails * Wrap `url` with `getPagePath` * Make `url` param mandatory in `prepareStyles` * Replace wrong `prefetch` words with `preload` * Fix failed to load message * Clarify the `element.sheet` check. * Wrap unit tests inside an additional `describe` * Remove unnecesary comment * Add TSDocs for helpers in styles.test.ts * Add missing periods in test comments * Normalize elements before calling `shortestCommonSupersequence` * Refactor `normalizeMedia` * Handle script modules * Install dynamic-importmap * Try with dynamic-importmap * Try ignore originally imported modules * Use a local copy of dynamic-importmap * Make it work * Cleaning up WIP * Add test files * Add an initial simple test * Partially fix pattern to intercept js requests * Add missing asset.php files * Try adding a module dependency * Disable module preloading * WIP Attempt code reorganization * WIP type Load objects * WIP Refactor types and add functions for preloading * Update router code * Add prefetch action to tests * Fix modules not being imported * Refactor script file and add TSDocs * Improve types and comments a bit * Improve types and TSDocs a little bit more * Add static dependencies to test modules * Make prefetch action asynchronous * Add failing test * Fix media preload when the target style list is empty * Update more tests * Fix how initial style sheets are detected * Detect first if the link has started prefetching * Add a failing test * Only enable/disable sheets that are ready * Improve failing test * Move styles and module preload to `regionsToVdom` * Add more comments * Remove try..catch around renderRegions call * Rename `prepareStyles` to `preloadStyles` * Start replacing modules term with script modules * Replace modules term with script modules in test files * Remove unnecessary promise around lexer.init * Remove old import fix from edge See https://github.com/guybedford/es-module-shims/pull/450/files#diff-d9f5f5cf3b132cd3d457a02a9202af63e78eb6b8b3284097e5d1cfd28b181e8eR351. * Remove unnecessary fetch pool * Simplify error messages in `fetchModule` * Do not add module type in `fetchModule` * Remove unused code * Catch `regionsToVdom` errors in `fetchPage` * Remove support for `import.meta` * Update package-lock * Ensure CSN checkers are hidden by default * Resolve inconsistencies and rename `markScriptModuleAsResolved` * Reset clientSideNavigation prop when navigation starts * Reapply "Remove the iAPI full-page experiment entirely (#70228)" This reverts commit 9cfe7ec. * WIP Move full-page logic to its own module * Fix interactivity-router TS with sub-packages * Needs to be composite project * Add empty files for good measure * Add package.json export * Remove extra whitespace * Remove exports property * Add full-page client-side navigation support * Import script modules before render * Turn php logic into a class * Change default mode priority to 9 * Fix init action in WP_Interactivity_API_Full_Page_Navigation * Prevent warning accessing to clientNavigationMode * Change body router region ID to core/body * Remove unnecessary `body` prop inside `regions` * Add a filter to enable/disable the experimental feature * Use `wp_body_open` instead of `wp_footer` * Move the experiment filter to `set_default_mode` * Remove full-page unlock * Only add the client navigation mode when full page is enabled * Move Full Page class to experimental folder * End buffer on `wp_footer` instead of `wp_body_open` * Enable the full-page CSN with a gutenberg experiment * Rename WP to Gutenberg in full-page CSN class * Remove "since" tag * Update changelog * Rename regionsToVdom and renderRegions to preparePage and renderPage * Make url param mandatori in preparePage * Remove unnecessary full-page mode from JS * Remove unnecessary full-page mode from PHP * Mention original code creators * Add descriptive names to props in ModuleLoad * Fix wrong JSON * Use Interactivity API instead of iAPI in experiment label * Avoid error messages in tests * Check the page title after navigation * Prevent warning about missing disable_server_directive_processing * Fix remaining ModuleLoad prop names * Simplify resolve function and remove unnecessary code --------- Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
chriszarate
pushed a commit
to chriszarate/gutenberg
that referenced
this pull request
Jul 1, 2025
…WordPress#69222) * Revert "iAPI Router: Fix CSS rule order in some constructed style sheets (WordPress#68923)" This reverts commit 4aad5e3. * Revert "iAPI Router: Handle styles assets on region-based navigation (WordPress#67826)" This reverts commit 12f7764. * Add changelog entry
chriszarate
pushed a commit
to chriszarate/gutenberg
that referenced
this pull request
Jul 1, 2025
…igation (WordPress#70353) * Revert "Remove the iAPI full-page experiment entirely (WordPress#70228)" This reverts commit f693991. * Reapply "iAPI Router: Handle styles assets on region-based navigation (WordPress#67826)" This reverts commit 0a5be06. * Reapply "iAPI Router: Fix CSS rule order in some constructed style sheets (WordPress#68923)" This reverts commit a53bb20. * Clone style elements and enable/disable them * Combine `media` and `disabled` * Add prefetch callback to style tests * Remove `async` from `applyStyles` * Add basic error handling * Rename `baseUrl` to `url` * Improve comment wrappers around new styles sheets * Minor fixes * Use a SCS algorithm to add new style sheets * Refactor and document `updateStylesWithSCS` * Fix promises returned by `updateStylesWithSCS` * First unit tests (claude) * Test improvements (claude) * Tests for shortestCommonSupersequence (claude) * Move scs tests a folder up * Refactor scs tests a bit * Refactor `updateStylesWithSCS` and `applyStyles` unit tests * Add new test for media attributes * Fix last appended elements * Rename and document `areStylesEqual` * Improve `areStyleEqual` TSDoc * Add more TSDocs * Add TSDoc for `shortestCommonSupersequence` * Test stylesheet loading fails * Wrap `url` with `getPagePath` * Make `url` param mandatory in `prepareStyles` * Replace wrong `prefetch` words with `preload` * Fix failed to load message * Clarify the `element.sheet` check. * Wrap unit tests inside an additional `describe` * Remove unnecesary comment * Add TSDocs for helpers in styles.test.ts * Add missing periods in test comments * Normalize elements before calling `shortestCommonSupersequence` * Refactor `normalizeMedia` * Handle script modules * Install dynamic-importmap * Try with dynamic-importmap * Try ignore originally imported modules * Use a local copy of dynamic-importmap * Make it work * Cleaning up WIP * Add test files * Add an initial simple test * Partially fix pattern to intercept js requests * Add missing asset.php files * Try adding a module dependency * Disable module preloading * WIP Attempt code reorganization * WIP type Load objects * WIP Refactor types and add functions for preloading * Update router code * Add prefetch action to tests * Fix modules not being imported * Refactor script file and add TSDocs * Improve types and comments a bit * Improve types and TSDocs a little bit more * Add static dependencies to test modules * Make prefetch action asynchronous * Add failing test * Fix media preload when the target style list is empty * Update more tests * Fix how initial style sheets are detected * Detect first if the link has started prefetching * Add a failing test * Only enable/disable sheets that are ready * Improve failing test * Move styles and module preload to `regionsToVdom` * Add more comments * Remove try..catch around renderRegions call * Rename `prepareStyles` to `preloadStyles` * Start replacing modules term with script modules * Replace modules term with script modules in test files * Remove unnecessary promise around lexer.init * Remove old import fix from edge See https://github.com/guybedford/es-module-shims/pull/450/files#diff-d9f5f5cf3b132cd3d457a02a9202af63e78eb6b8b3284097e5d1cfd28b181e8eR351. * Remove unnecessary fetch pool * Simplify error messages in `fetchModule` * Do not add module type in `fetchModule` * Remove unused code * Catch `regionsToVdom` errors in `fetchPage` * Remove support for `import.meta` * Update package-lock * Ensure CSN checkers are hidden by default * Resolve inconsistencies and rename `markScriptModuleAsResolved` * Reset clientSideNavigation prop when navigation starts * Reapply "Remove the iAPI full-page experiment entirely (WordPress#70228)" This reverts commit 9cfe7ec. * WIP Move full-page logic to its own module * Fix interactivity-router TS with sub-packages * Needs to be composite project * Add empty files for good measure * Add package.json export * Remove extra whitespace * Remove exports property * Add full-page client-side navigation support * Import script modules before render * Turn php logic into a class * Change default mode priority to 9 * Fix init action in WP_Interactivity_API_Full_Page_Navigation * Prevent warning accessing to clientNavigationMode * Change body router region ID to core/body * Remove unnecessary `body` prop inside `regions` * Add a filter to enable/disable the experimental feature * Use `wp_body_open` instead of `wp_footer` * Move the experiment filter to `set_default_mode` * Remove full-page unlock * Only add the client navigation mode when full page is enabled * Move Full Page class to experimental folder * End buffer on `wp_footer` instead of `wp_body_open` * Enable the full-page CSN with a gutenberg experiment * Rename WP to Gutenberg in full-page CSN class * Remove "since" tag * Update changelog * Rename regionsToVdom and renderRegions to preparePage and renderPage * Make url param mandatori in preparePage * Remove unnecessary full-page mode from JS * Remove unnecessary full-page mode from PHP * Mention original code creators * Add descriptive names to props in ModuleLoad * Fix wrong JSON * Use Interactivity API instead of iAPI in experiment label * Avoid error messages in tests * Check the page title after navigation * Prevent warning about missing disable_server_directive_processing * Fix remaining ModuleLoad prop names * Simplify resolve function and remove unnecessary code --------- Co-authored-by: DAreRodz <darerodz@git.wordpress.org> Co-authored-by: luisherranz <luisherranz@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds support for style assets on region-based navigation, allowing stylesheets to be applied or removed when navigating to a new page with different styles.
Why?
This feature is required for some core blocks like the Comments one. In this particular case, when publishing a new comment on a post that does not have any comments, we want to display the new comments without reloading the page using
navigate(). The new comment might require styles not present on the initial post page.How?
The implementation uses the
adoptedStyleSheetsAPI.For each
<style>and<link rel=stylesheet>tags, aCSSStyleSheetis generated. If the CSS code contains a relative URL pointing to another asset (e.g., images or fonts), the URL is converted into an absolute URL. That could be removed once thebaseURLoption is supported in Chrome, Edge, or Opera (see coverage in MDN).These
CSSStyleSheetinstances are then passed to theadoptedStyleSheetsproperty in the correct order.With these APIs, we can solve the following problems:
The PR also replaces the current implementation we have for full-site navigation.
Testing Instructions
Important: you could notice elements outside the Query block that their appearance change. It's related to this issue, and will be addressed in a subsequent PR.