MWPW-167716 Remove await from both gnav and georouting #3680
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
Commits
|
|
sharmrj
requested review from
bandana147,
Deva309,
sonawanesnehal3,
overmyheadandbody,
mokimo and
vhargrave
| } | ||
| const header = document.querySelector('header'); | ||
| if (header) { | ||
| header.classList.add('gnav-hide'); | ||
| await loadBlock(header); | ||
| loadBlock(header); |
There was a problem hiding this comment.
Is there any influence on CLS if you do this? Also what happens in terms of personalization? What if that loads before the GNAV?
There was a problem hiding this comment.
No there's no CLS.
AFAICT if personalization must load it will load before either gnav or georouting; either right before georouting in loadPostLCP or in checkForPageMods. In both cases it is awaited. There is some personalization that takes place inside the gnav, but simply removing the await does not affect it in any way.
| @@ -1192,13 +1192,15 @@ async function loadPostLCP(config) { | |||
|
|
|||
| const georouting = getMetadata('georouting') || config.geoRouting; | |||
| if (georouting === 'on') { | |||
| const { default: loadGeoRouting } = await import('../features/georoutingv2/georoutingv2.js'); | |||
| await loadGeoRouting(config, createTag, getMetadata, loadBlock, loadStyle); | |||
| import('../features/georoutingv2/georoutingv2.js') | |||
There was a problem hiding this comment.
Given @vhargrave concerns in the discussion, I suppose this was brought up with the georouting PM?
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
|
To give an idea of what this change accomplishes, it essentially schedules our network requests postLCP more efficiently. As a visual representation, consider the below screen shot of our page load currently. 
We see that many network requests are chained, blocking further requests until they are received. Specifically there's a visible bottleneck in the middle there. That happens to be georouting and global-navigation. See the trace here: Now with the change in this PR: 
trace:
Trace-20250214T092719.json.zip We can see that the bottleneck is gone. By removing just these two |
| @@ -1192,13 +1192,15 @@ async function loadPostLCP(config) { | |||
|
|
|||
| const georouting = getMetadata('georouting') || config.geoRouting; | |||
| if (georouting === 'on') { | |||
| const { default: loadGeoRouting } = await import('../features/georoutingv2/georoutingv2.js'); | |||
| await loadGeoRouting(config, createTag, getMetadata, loadBlock, loadStyle); | |||
| import('../features/georoutingv2/georoutingv2.js') | |||
There was a problem hiding this comment.
If you haven't done the performance enhancement we discussed for the georouting, then please confirm that this delay was tested and discussed with the georouting PM.
Otherwise please at least do the enhancement that was discussed (kicking off the loading of the .json file here).
There was a problem hiding this comment.
We've tagged them on JIRA, and we'll continue the conversation there.
I looked into doing that optimization here, but I think it would fit well as its own PR. I'd prefer to keep the PRs as granular as we can. If you prefer I can make it the very next optimization we do.
There was a problem hiding this comment.
I understand that, but I can't approve a change to the load time of the georouting that goes live if you haven't approved it with the georouting PM or mitigated it with the change we discussed.
There was a problem hiding this comment.
Ok I've raised a separate PR to optimize georouting first and then we can merge this one. Here's the PR @vhargrave : #3720
| } | ||
| const header = document.querySelector('header'); | ||
| if (header) { | ||
| header.classList.add('gnav-hide'); | ||
| await loadBlock(header); | ||
| loadBlock(header); | ||
| header.classList.remove('gnav-hide'); | ||
| } | ||
| loadTemplate(); |
There was a problem hiding this comment.
Not a must, but i bet you could kick the loading of the fonts.js at the beginning of the postLCP somewhere and only await it here.. That would probably also give you some great savings.
This is the
firstsecond offourfive PRs (the first is #3720) aimed at improving Global-Navigation performance. This first PR does not directly affect the global-navigation (or georouting), but is rather aimed at improving the load times of the sections after the first one. For more information look at the discussion #3558The first section is assumed to be LCP and is prioritized. After the first section we load a series of features/blocks, among them georouting and global-navigation. Since we
awaitboth of these blocks, the subsequent sections cannot continue until they stop blocking the main thread.This can be bad because the user cannot see the rest of the page, or even scroll, until the subsequent sections load. The affect of this poor UX is not something we measure but it is important nonetheless. And even though we assume LCP should be in the first section, in practice it is not always.
The solution is then to remove the
awaits on the global-navigation and georouting. The effects of this change have been documented in the discussion #3558.Resolves: MWPW-167716
Test URLs: