MWPW-154899 - Pilot preloading marquee dependencies #383
Conversation
* Preload known marquee dependencies if in LCP section * Not in scope: MEP awareness Resolves: MWPW-154899
|
There was a problem hiding this comment.
Should we wait for @chrischrischris PR adobecom/milo#2686 in milo core first?
That way we might get more accurate data.
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
| loadLink(`${miloLibs}/blocks/marquee/marquee.css`, { rel: 'stylesheet' }); | ||
| loadLink(`${miloLibs}/blocks/marquee/marquee.js`, { as: 'script', rel: 'modulepreload' }); | ||
| loadLink(`${miloLibs}/utils/decorate.js`, { as: 'script', rel: 'modulepreload' }); | ||
| } |
There was a problem hiding this comment.
@auniverseaway
The changes currently rely on the assumption that the default authoring of the first block on the page is marquee and it will always be marquee
In a case if there is a marquee on page with mep personalization that changes it to hero-marquee..
marquee js and css would get loaded
but then mep will change it
milo will run as usual and hero-marquee js and css will get loaded
So in this scenario extra call was made to a js or css that was not needed
There was a problem hiding this comment.
The changes currently rely on the assumption that the default authoring of the first block on the page is marquee and it will always be marquee
This isn't quite accurate, but I think I know where you're going with your question. FWIW, this does not assume marquee is the first block on a page. Only that there is a marquee somewhere in the first section. If marquee is not in the first section, this does nothing and that is the intention.
In a case if there is a marquee on page with mep personalization that changes it to hero-marquee..
marquee js and css would get loaded
but then mep will change it
milo will run as usual and hero-marquee js and css will get loaded
So in this scenario extra call was made to a js or css that was not needed
Yes. This is expected. Hence, "pilot" and "out of scope: mep awareness" as by the time MEP fires, this would not provide any value. It's too late in the order of operations. The ideal use case is that MEP replaces a same block for same block, but with different content. This is not meant to cover all edge cases, just enough to collect some data to see if this improves performance.
There was a problem hiding this comment.
@aishwaryamathuria we want to trial this as part of the performance tiger team, we are closely observing the numbers and want to see if this has any positive benefit.
As Chris mentioned, if this assumption (marquee in the first section) is not true, there isn't a lot of harm. Would be awesome if we could take it forward, if the concept works on CC we might want to adopt it more broadly in milo-core.
if its doing nothing, we can remove the code again.
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
7 similar comments
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
I'd recommend
@salonijain3 can someone take care of this? |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
3 similar comments
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
@sharath-kannan can you look at improving the message logic? It's a little spammy if the PRs don't get merged
|
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to failing checks |
Sure, i will check this. |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to failing checks |
2 similar comments
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to failing checks |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to failing checks |
|
@mokimo @salonijain3 is there anything needed from me on this? The lana tracking PR has been merged. Would you like me to assign someone to verify this? |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
1 similar comment
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
I don't know much about the cc contribution process. @salonijain3 can you help getting this verified and merged? The goal are some performance improvements we'd want to trial on cc. |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
2 similar comments
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Skipped merging 383: MWPW-154899 - Pilot preloading marquee dependencies due to missing verified label. kindly make sure that the PR has been verified |
|
Given the concerns from @aishwaryamathuria this is currently not in a state to be merged. @Ruchika4 would you want to give this a spin? I'll close this for now given there's no path forward. |
Resolves: MWPW-154899
Test URLs:
How to test:
Observe in the network calls that all marquee dependencies load before things that are historically in front of blocks: placeholders, etc.
Additional context:
This is a pilot to see if this feature should be elevated to Milo at a later date. I'm a little suspect this will add any performance gains in low bandwidth scenarios, but it's worth trying.