MWPW-154898 No wait for modal fragment

[TsayAdobe]

• A section doesn't need to wait for loading of modal fragments. Marquee and modal can be loaded at the same time.
• This PR is one part of MWPW-152286 Optimize marquee loading #2493, which has been closed. The other part will be addressed in MWPW-154899

Resolve: MWPW-154898

Test URLs:

Before: https://main--milo--adobecom.hlx.live/drafts/nala/blocks/marquee/marquee-light?martech=off
After: https://mwpw-154898--milo--adobecom.hlx.live/drafts/nala/blocks/marquee/marquee-light?martech=off

DC Test URLs:

Before: https://www.stage.adobe.com/acrobat/how-to/pdf-editor-pdf-files.html
Marquee and icons are loaded after modal.

After: https://www.stage.adobe.com/acrobat/how-to/pdf-editor-pdf-files.html?milolibs=mwpw-154898
Marquee, icons, and modal are loaded at the same time.

[aem-code-sync]

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

• Re-run PSI checks
• Re-sync branch

Commits

• d9acef4 ✅ (latest)

[aem-code-sync]

Page	Scores	Audits	Google
/drafts/nala/blocks/marquee/marquee-light?martech=off

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.66%. Comparing base (58a1fdf) to head (d9acef4).
Report is 50 commits behind head on stage.

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #2607      +/-   ##
==========================================
- Coverage   95.67%   95.66%   -0.01%     
==========================================
  Files         169      169              
  Lines       45179    45191      +12     
==========================================
+ Hits        43224    43232       +8     
- Misses       1955     1959       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mokimo]

LGTM, however I don't have the most context on this portion of the utils.

@chrischrischris Could you have a look? We might get away with just filtering the modals out

[mokimo]

I would suggest we don't export it unless we have a concrete use-case for using it outside of this module.
Reason being, once exported, it's a lot harder to ever remove or change it again as consumers might use it and we don't do any breaking changes in milo.

Suggested change

	export function partition(arr, fn) {
	return arr.reduce(
	(acc, val, i, ar) => {
	acc[fn(val, i, ar) ? 0 : 1].push(val);
	return acc;
	},
	[[], []],
	);
	}
	const partition = (arr, fn) =>
	arr.reduce(
	(acc, val, i, ar) => {
	acc[fn(val, i, ar) ? 0 : 1].push(val);
	return acc;
	},
	[[], []],
	);

[TsayAdobe]

It's easier for unit tests with export. Testability is important.

[mokimo]

@mirafedas can you confirm #2094 still works?

[mokimo]

If we don't ever need to wait for modals, should we just filter them before they even land in the preload section and let them load normally later on?
So in my mind the fix is filtering modals completely from here: https://github.com/adobecom/milo/blob/stage/libs/utils/utils.js#L798

[github-actions]

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.

[chrischrischris]

I don't think it will affect it, but has this been tested with delayed modals?

[TsayAdobe]

I don't think it will affect it, but has this been tested with delayed modals?

There is a unit test. It passed.

milo/test/blocks/modals/modals.test.js

Lines 201 to 217 in c00c777

	it('shows the modal with a delay, and remembers it was shown on this page', async () => {
	window.sessionStorage.removeItem('shown:#delayed-modal');
	const anchor = document.createElement('a');
	anchor.setAttribute('data-modal-path', '/fragments/promos/fragments/cc-all-apps-promo-full-bleed-image');
	anchor.setAttribute('data-modal-hash', '#delayed-modal:delay=1');
	document.body.appendChild(anchor);
	expect(delayedModal(anchor)).to.be.true;
	await delay(1000);
	const modal = await waitForElement('#delayed-modal');
	expect(modal).to.be.not.null;
	expect(document.querySelector('#delayed-modal').classList.contains('delayed-modal'));
	expect(window.sessionStorage.getItem('shown:#delayed-modal').includes(window.location.pathname)).to.be.true;
	expect(window._satellite.track.called).to.be.true;
	window.sessionStorage.removeItem('shown:#delayed-modal');
	modal.remove();
	anchor.remove();
	});

[chrischrischris]

I know that this is probably the most efficient implementation of partition, but I think that considering the arrays being partitioned will always be small, I think readability should take precedence. Also I think any testing of it can be part of general unit tests given it's simplicity.

cc @mokimo

Suggested change

	export function partition(arr, fn) {
	return arr.reduce(
	(acc, val, i, ar) => {
	acc[fn(val, i, ar) ? 0 : 1].push(val);
	return acc;
	},
	[[], []],
	);
	}
	function partition(arr, fn) {
	return [
	arr.filter(fn),
	arr.filter((el, i) => !fn(el, i)),
	];
	}

[mokimo]

+1ing @chrischrischris opinion

[TsayAdobe]

The function name partition is clear and accurately describes its purpose. It already possesses sufficient readability.

[sharmrj]

There could be a middle ground to make it more readable similar to the haskell implementation

function partition(arr, predicate) {
    const select = ([ts, fs], x, ...args) => {
      if (predicate(x, ...args)) {
        ts.push(x);
      } else {
        fs.push(x);
      }
      return [ts, fs];
    };
    
  return arr.reduce(select, [[], []]);
}

The select tells the reader that we're folding over the array and 'selecting' the values matching the predicate, so the really only need to read the last line to understand the function.
alternatively you might find this clearer (select isn't a closure)

const select = (predicate) => ([ts, fs], x, ...args) => {
  predicate(x, ...args) ? ts.push(x) : fs.push(x);
  return [ts, fs];
}

const partition = (arr, predicate) => arr.reduce(select(predicate), [[], []]);

[github-actions]

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.