-
Notifications
You must be signed in to change notification settings - Fork 183
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
MWPW-167717 Parallelize requests inside global-navigation.js #3865
base: stage
Are you sure you want to change the base?
Conversation
… on each test to work correctly with the performance optimizations
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
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. |
@@ -847,7 +847,7 @@ function decorateDefaults(el) { | |||
|
|||
export async function getGnavSource() { | |||
const { locale, dynamicNavKey } = getConfig(); | |||
let url = getMetadata('gnav-source') || `${locale.contentRoot}/gnav`; | |||
let url = getMetadata('gnav-source') || `${locale?.contentRoot ?? window.location.origin}/gnav`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need window.location.origin
, any page would always have locale.contentRoot, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily: some unit tests were failing because of this, which made me aware of the fact that it's possible, in certain situations, to have no locale.contentRoot
. Defensive code, in general, is better than code that makes assumptions that can be wrong as it allows us to fail gracefully.
@@ -994,13 +1026,13 @@ class Gnav { | |||
const promoPath = getMetadata('gnav-promo-source'); | |||
const fedsPromoWrapper = document.querySelector('.feds-promo-aside-wrapper'); | |||
|
|||
if (!promoPath) { | |||
if (!promoPath || !asideJsPromise) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the !promoPath
since it is already checked in asideJsPromise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to leave this code a bit defensive, even if it's just for the sake of unit tests. If ever there arises a situation where asideJSPromise
is null but promoPath
exists, we'll still get a promo.
loadStyle(`${url}base.css`, () => { | ||
if (getConfig().theme === 'dark') loadStyle(`${url}dark-nav.css`); | ||
}); | ||
})(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nishantka Can you check this once? I remember you encountered an issue while implementing this and had to explicitly use a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work, we just have to load dark-nav.css
after base.css
. We were using callback for adding Lana logs here - if we can catch errors also that will be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bandana147 @nishantka I've added some error handling here.
…timize-network-requests
… to rejecting a promise
…timize-network-requests
This is the third in a series of PRs to optimize the global-navigation load time.
See the discussion #3558 for details, including the results of lab performance tests. Some discussion was had whether the parallelization of calls should go in utils.js instead of global-navigation.js, but it was ultimately decided that we did not want to add too much code to utils.js.
This is the first PR that directly touches gnav code
utils.js
, which is expected to already have loaded in both Milo and in Standalone contexts, the requests for all required dependencies are kicked off concurrently instead of one at a time. This was done by changing the import statements at the top of the file into dynamic imports.header
element has the classhas-breadcrumbs
, which is added indecorateHeader
insideutils.js
when abreadcrumbs
block is in the document.gnav-promo-source
is present in thehead
.plain.html
document was also lifted to the beginning of the file.fetchAndProcessPlainHTML
was modified to optionally accept a promise.getGnavSource
.global-navigation.js
in tests where it is required rather than as a top level import, and 2) to add a unique query parameter to cause the module to reload completely.Resolves: MWPW-167717
Test URLs:
QA:
A vanilla page: https://main--cc--adobecom.aem.live/products/photoshop?milolibs=gnav-optimize-network-requests
Standalone Dark Mode: https://adobecom.github.io/nav-consumer/navigation.html?env=prod&navbranch=gnav-optimize-network-requests&theme=dark
Standalone Light mode: https://adobecom.github.io/nav-consumer/navigation.html?env=prod&navbranch=gnav-optimize-network-requests
We should also test a page with a promo, and a page with the dynamic nav enabled.
If performance is being tested, it's best to use content overrides on prod and compare it to prod.