Skip to content
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.

Sign up for GitHub

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-168334 Load georoutingv2.json and georoutingv2.js in parallel #3720

Open
wants to merge 4 commits into
base: stage
Choose a base branch
from
+46 −41
Open

MWPW-168334 Load georoutingv2.json and georoutingv2.js in parallel #3720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fe636d
load georoutingv2.json in parallel with georoutingv2.js
sharmrj Feb 21, 2025
5312882
remove performance marks
sharmrj Feb 21, 2025
4629c38
fixed unit tests
sharmrj Feb 21, 2025
1e83df7
Merge branch 'stage' of https://github.com/adobecom/milo into optimiz…
sharmrj Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions libs/features/georoutingv2/georoutingv2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ export default async function loadGeoRouting(
getMetadataFunc,
loadBlockFunc,
loadStyleFunc,
v2jsonPromise,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give this the default value of a promise fetch to the georoutingV2.json file?

) {
if (getGeoroutingOverride()) return;
config = conf;
Expand All @@ -279,25 +280,22 @@ export default async function loadGeoRouting(
loadBlock = loadBlockFunc;
loadStyle = loadStyleFunc;

const urls = [
`${config.contentRoot ?? ''}/georoutingv2.json`,
`${config.contentRoot ?? ''}/georouting.json`,
`${getFederatedContentRoot()}/federal/georouting/georoutingv2.json`,
];
let resp;
for (const url of urls) {
resp = await fetch(url);
if (resp.ok) {
if (url.includes('georouting.json')) {
const json = await resp.json();
// eslint-disable-next-line import/no-cycle
const { default: loadGeoRoutingOld } = await import('../georouting/georouting.js');
loadGeoRoutingOld(config, createTag, getMetadata, json);
}
break;
}
}
const json = await resp.json();
const loadOldGeorouting = async (json) => {
const { default: loadGeoRoutingOld } = await import('../georouting/georouting.js');
await loadGeoRoutingOld(config, createTag, getMetadata, json);
return 'use-old-georouting';
};
const oldGeorouting = () => fetch(`${config.contentRoot ?? ''}/georouting.json`)
.then((r) => r.json())
.then(loadOldGeorouting)
.catch(() => null);
const federatedJSON = () => fetch(`${getFederatedContentRoot()}/federal/georouting/georoutingv2.json`)
.then((r) => r.json())
.catch(() => null);

const json = (await v2jsonPromise) ?? (await oldGeorouting()) ?? (await federatedJSON());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're kind of cramming multiple asynchronous operations and fallback logic into a single expression here. I'd separate this out for readability.

if (json === 'use-old-georouting') return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would really recommend avoiding things like magic strings - https://softwareengineering.stackexchange.com/questions/365339/what-is-wrong-with-magic-strings


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my upper comments, I'd maybe even just edit to something simpler like so (this is untested btw). Your call on how you want to do it though:

  let json;
  const response = await v2jsonPromise;
  if(response.ok) json = await response.json();

  if (!json) {
    // Try old georouting
    const r = await fetch(`${config.contentRoot ?? ''}/georouting.json`);
    if (r.ok) {
      const data = await r.json();
      const { default: loadGeoRoutingOld } = await import('../georouting/georouting.js');
      await loadGeoRoutingOld(config, createTag, getMetadata, data);
      return;
    }
  }
  if(!json) {
    const r = await fetch(`${getFederatedContentRoot()}/federal/georouting/georoutingv2.json`);
    if (r.ok) {
      json = await r.json();
    }
  }
  if (!json) return;

const { locale } = config;

const urlLocale = locale.prefix.replace('/', '');
Expand Down
5 changes: 4 additions & 1 deletion libs/utils/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,8 +1250,11 @@ async function loadPostLCP(config) {

const georouting = getMetadata('georouting') || config.geoRouting;
if (georouting === 'on') {
const jsonPromise = fetch(`${config.contentRoot ?? ''}/georoutingv2.json`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep this even leaner , you could probably only call the fetch right? (without any .then or .catch handling)

.then((r) => r.json())
.catch(() => null);
const { default: loadGeoRouting } = await import('../features/georoutingv2/georoutingv2.js');
await loadGeoRouting(config, createTag, getMetadata, loadBlock, loadStyle);
await loadGeoRouting(config, createTag, getMetadata, loadBlock, loadStyle, jsonPromise);
}
const header = document.querySelector('header');
if (header) {
Expand Down
46 changes: 25 additions & 21 deletions test/features/georoutingv2/georoutingv2.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ const ogInnerHeight = window.innerHeight;
const ogFetch = window.fetch;
window.fetch = stub();

const v2JSONPromise = () => fetch('/georoutingv2.json')
.then((r) => r.json())
.catch(() => null);

function stubHeadRequestToReturnVal(prefix, val) {
const path = window.location.href.replace(`${window.location.origin}`, '');
window.fetch.withArgs(`${prefix}${path}`, { method: 'HEAD' }).returns(
Expand Down Expand Up @@ -304,7 +308,7 @@ describe('GeoRouting', () => {

it('Does create a modal if detected country from IP is CH and url prefix is US', async () => {
// prepare
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.not.be.null;
Expand All @@ -314,7 +318,7 @@ describe('GeoRouting', () => {
// prepare
setUserCountryFromIP('US');
document.cookie = 'international=us;path=/;';
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.be.null;
Expand All @@ -325,7 +329,7 @@ describe('GeoRouting', () => {
it('Shows no modal if aiming for CH page and IP in Switzerland', async () => {
// prepare
mockConfig.locale.prefix = 'ch_de';
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.be.null;
Expand All @@ -335,7 +339,7 @@ describe('GeoRouting', () => {

it('If aiming for US page but IP in Switzerland shows CH links and US continue', async () => {
// prepare
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.not.be.null;
Expand Down Expand Up @@ -367,7 +371,7 @@ describe('GeoRouting', () => {
it('If aiming for US page but IP in Egypt arabic content in geo routing modal is in rtl', async () => {
// prepare
setUserCountryFromIP('EG');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.not.be.null;
Expand All @@ -387,7 +391,7 @@ describe('GeoRouting', () => {
it('If aiming for US page but IP in Egypt english content in geo routing modal is in ltr', async () => {
// prepare
setUserCountryFromIP('EG');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.not.be.null;
Expand All @@ -408,7 +412,7 @@ describe('GeoRouting', () => {
// prepare
mockConfig.locale.prefix = 'eg_ar';
setUserCountryFromIP('US');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
const wrapper = document.querySelector('.georouting-wrapper');
// assert
Expand All @@ -425,7 +429,7 @@ describe('GeoRouting', () => {
mockConfig.locale.prefix = '/ch_fr';
setUserCountryFromIP('US');
document.cookie = 'international=de;path=/;';
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const wrapper = document.querySelector('.georouting-wrapper');
// assert
const swissFrenchData = mockGeoroutingJson.georouting.data.find((d) => d.prefix === 'ch_fr');
Expand All @@ -443,7 +447,7 @@ describe('GeoRouting', () => {
// prepare
mockConfig.locale.prefix = 'mena_en';
setUserCountryFromIP('BW');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const wrapper = document.querySelector('.georouting-wrapper');
// assert
const tabs = wrapper.querySelectorAll('.tabpanel');
Expand All @@ -464,7 +468,7 @@ describe('GeoRouting', () => {
mockConfig.locale.prefix = 'mena_en';
setUserCountryFromIP('BW');
document.cookie = 'international=ch_de;path=/';
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const wrapper = document.querySelector('.georouting-wrapper');
// assert
const tabs = wrapper.querySelectorAll('.tabpanel');
Expand All @@ -484,7 +488,7 @@ describe('GeoRouting', () => {
// prepare
mockConfig.locale.prefix = 'ch_de';
document.cookie = 'international=ch_fr;path=/;';
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
mockConfig.locale.prefix = '';
Expand All @@ -494,7 +498,7 @@ describe('GeoRouting', () => {
it('If IP is from an unknown country no modal is show', async () => {
// prepare
setUserCountryFromIP('NOEXIST');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.be.null;
Expand All @@ -506,7 +510,7 @@ describe('GeoRouting', () => {
// prepare
stubFallbackMetadata('off');
stubHeadRequestToReturnVal('/ch_it', false);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
const tabs = modal.querySelectorAll('.tabpanel');
Expand All @@ -533,7 +537,7 @@ describe('GeoRouting', () => {
it('Will show fallback links if fallbackrouting is on and page exist request fails', async () => {
// prepare
stubHeadRequestToReturnVal('/ch_it', false);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.not.be.null;
Expand All @@ -549,7 +553,7 @@ describe('GeoRouting', () => {
stubHeadRequestToReturnVal('/ch_de', false);
stubHeadRequestToReturnVal('/ch_it', false);
stubHeadRequestToReturnVal('/ch_fr', false);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.be.null;
Expand All @@ -561,7 +565,7 @@ describe('GeoRouting', () => {
});

it('Closes picker if picker open and then clicking somewhere else within the modal', async () => {
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
const cookie = getCookie('international');
// assert
Expand All @@ -582,7 +586,7 @@ describe('GeoRouting', () => {

it('Add class .top to picker when there is no space to render below the trigger button', async () => {
await setViewport({ width: 600, height: 100 });
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
const links = modal.querySelectorAll('a');
links[0].click();
Expand All @@ -593,7 +597,7 @@ describe('GeoRouting', () => {

it('Sets international and georouting_presented cookies on link click in modal', async () => {
// prepare
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
const cookie = getCookie('international');
// assert
Expand All @@ -609,7 +613,7 @@ describe('GeoRouting', () => {
it('Does not open georouting modal if georouting hide is active', async () => {
// prepare
setGeorouting('off');
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
// assert
expect(modal).to.be.null;
Expand All @@ -621,15 +625,15 @@ describe('GeoRouting', () => {
stubFetchForGeorouting(false);
stubFetchForGeoroutingOld(false);
stubFetchForFederalGeorouting();
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
expect(modal).to.not.be.null;
});

it('Will load old georouting modal if georoutingv2 is not found', async () => {
stubFetchForGeorouting(false);
stubFetchForGeoroutingOld(true);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle);
await init(mockConfig, createTag, getMetadata, loadBlock, loadStyle, v2JSONPromise());
const modal = document.querySelector('.dialog-modal');
expect(modal).to.not.be.null;
});
Expand Down
Loading