iAPI Router: Handle styles assets on region-based navigation #67826
Conversation
DAreRodz
added
[Type] Enhancement
|
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. |
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.
| 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
|
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.