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.

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-153962: Introduce maslibs query parameter #2544

Merged
merged 14 commits into from
Jul 10, 2024
2 changes: 1 addition & 1 deletion head.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<meta name="viewport" content="width=device-width, initial-scale=1"/>
<script src="/libs/scripts/scripts.js" type="module" crossorigin="use-credentials"></script>
<script src="/libs/scripts/scripts.js" type="module"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember the details, but this file was previously heavily cached and making changes to it lead to various issues. Are you certain this should be removed? Why do Milo's scripts interfere with Merch logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

is the use case to render a milo.adobe.com page with different commerce source, @yesil ? may be we could make that change a separate one as i feel like it's not going to be an easy one and we can still play around with most commerce pages on CC & DC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With crossorigin="use-credentials" all the subsequent js module loaded by /libs/scripts/scripts.js will be subject to CORS restrictions. Since maslibs param will allow to load commerce.js from a different domain instead of libs/deps folder, this will fail. With crossorigin="use-credentials" we cannot use Access-Control-Allow-Origin:* on commerce.js neither, it must be an explicit domain, without wildcards.

I don't know changing this file will causes an immediate cache flush, but we don't need it immediately. We can start using it once CDN caches are naturally refreshed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@npeltier we have plenty of test pages under milo and we need the ability to test them with maslibs="stage" for example.

I think for CC and DC we will be fine since they don't have this crossorigin restriction on the script module.

Copy link
Contributor

Choose a reason for hiding this comment

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

we discussed this offline and agreed we can move this to a second one line PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this change in a separate PR: #2549

Copy link
Contributor Author

@yesil yesil Jul 9, 2024

Choose a reason for hiding this comment

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

I think I made a mistake in my tests, will close #2549 for now.
This PR is still good to go.

<link rel="stylesheet" href="/libs/styles/styles.css"/>
<link rel="icon" href="data:," size="any"/>
58 changes: 48 additions & 10 deletions libs/blocks/merch/merch.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,34 @@
let log;
let upgradeOffer = null;

/**
* Given a url, calculates the hostname of MAS platform.
* Supports, www prod, stage, local and feature branches.
* @param {string} hostname
* @param {string} maslibs
* @returns base url for mas platform
*/
export function getMasBase(hostname, maslibs) {
narcis-radu marked this conversation as resolved.
Show resolved Hide resolved
let { baseUrl } = getMasBase;
Copy link
Contributor

Choose a reason for hiding this comment

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

this would always be empty on the first pass, right? I think our pattern here is to expose a let baseUrl outside the method and then update the value. https://github.com/adobecom/milo/pull/2544/files#diff-01476d99c8ba4d00b7e767d0afbc424902d0d40e75c4ec4272a966edd0d4124dR149 is a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used this pattern in the other functions in merch block. Thus, we can cache the calculated value, and erase it when needed (mostly in tests). Using a module scope variable makes testing harder. You need to provide a utility method to reset the module scope variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

But still, on the first pass the baseUrl value would be empty and I don't see a reason why you would need to call the method multiple times.

Copy link
Contributor Author

@yesil yesil Jul 10, 2024

Choose a reason for hiding this comment

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

@narcis-radu it is not going to be called to many times but several times definitely.
later after commerce.js, we will start adding maslibs feature to merch-card, merch-card-collection and other merch blocks as well.
merch being auto block it will be the first one to call this method and have the base url initialised, then the other blocks can use the value.
@3ch023 I'm tempted to add merch-card in this PR, thus we can give a better shape to the code, wdyt? cc: @npeltier

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not happy with the baseUrl approach since it doesn't follow the pattern used in other Milo blocks, but I don't think this is a blocker or reason to request changes. Thanks for addressing the important changes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, something like getMasLibs setMasLibs would be better. I'll give it a try tomorrow my morning.

if (baseUrl) return baseUrl;
baseUrl = 'https://www.adobe.com/mas';
if (!maslibs) return baseUrl;
if (maslibs === 'stage') {
baseUrl = 'https://www.stage.adobe.com/mas';
} else if (maslibs === 'local') {
baseUrl = 'http://localhost:8000';
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be that people use other ports. I, for example, am always using :6456, having this hardcoded might limit its usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6456 is the Milo port to use with milolibs=local, and it is hard coded in Milo consumer apps (e.g: https://github.com/adobecom/cc/blob/stage/creativecloud/scripts/utils.js#L39)

I proposed http://localhost:8000 for using MAS locally with maslibs=local, but we can use a different port if 8000 is a reserved port.

Copy link
Contributor

Choose a reason for hiding this comment

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

didn't find any obvious tool using 8000 port so i'm good with it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

most of the quick local dev servers (e.g. python http.server) use 8000 as the default port and we might have some conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for tacocat we used to use ports 9001-9008 for different modules.
I'll pick 9001 :)

} else {
const extension = /.live$/.test(hostname) ? 'live' : 'page';
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to revisit this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

We will have to discuss offline

if (/--/.test(maslibs)) {
baseUrl = `https://${maslibs}.hlx.${extension}`;
} else {
baseUrl = `https://${maslibs}--mas--adobecom.hlx.${extension}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about we make 'maslibs' work the same way as 'milolibs' do?

?maslibs=mwpw-153962--mas--yesil
branch-fork-owner
Then you can save precious bytes and remove stage and prod cases, since the link could be given as
?maslibs=main--mas--adobecom or ?maslibs=stage--mas--adobecom (when/if we have stage) instead of ?maslibs=stage

I know the latter is shorter but it involves an additional learning curve for devs and IMO is not worth the space it takes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During stage testing, I think just using &maslibs=stage instead of &maslibs=stage--mas--adobecom will save a lot of time to humans.
But saving bytes is also interesting. @npeltier @narcis-radu @3ch023 @overmyheadandbody
👍 or 👎 for saving bytes?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think i prefer mariia/milolibs's approach, yes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
}
getMasBase.baseUrl = baseUrl;
return baseUrl;
}

Check warning on line 151 in libs/blocks/merch/merch.js

View check run for this annotation

Codecov / codecov/patch

libs/blocks/merch/merch.js#L133-L151

Added lines #L133 - L151 were not covered by tests

export async function polyfills() {
if (polyfills.promise) return polyfills.promise;
let isSupported = false;
Expand Down Expand Up @@ -326,15 +354,18 @@
}

export async function getCheckoutAction(offers, options, imsSignedInPromise) {
const [downloadAction, upgradeAction, modalAction] = await Promise.all([
getDownloadAction(options, imsSignedInPromise, offers),
getUpgradeAction(options, imsSignedInPromise, offers),
getModalAction(offers, options),
]).catch((e) => {
try {
await imsSignedInPromise;
const [downloadAction, upgradeAction, modalAction] = await Promise.all([
getDownloadAction(options, imsSignedInPromise, offers),
getUpgradeAction(options, imsSignedInPromise, offers),
getModalAction(offers, options),
]);
return downloadAction || upgradeAction || modalAction;
} catch (e) {
log?.error('Failed to resolve checkout action', e);
return [];
});
return downloadAction || upgradeAction || modalAction;
}
}

/**
Expand All @@ -349,7 +380,13 @@
const { env, commerce = {}, locale } = getConfig();
commerce.priceLiteralsPromise = fetchLiterals(PRICE_LITERALS_URL);
initService.promise = initService.promise ?? polyfills().then(async () => {
const commerceLib = await import('../../deps/commerce.js');
const { hostname, searchParams } = new URL(window.location.href);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the method itself get the hostname and searchParams? What's the value of sending these from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept getMasBase as a pure function so that it is easier to test it.
Since we need to check whether maslibs param is present here and getMasBase only needs hostname and maslibs parameters, I limited it to those params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will address this in a different PR.

const maslibs = searchParams.get('maslibs');
let commerceLibPath = '../../deps/commerce.js';
if (maslibs) {
commerceLibPath = `${getMasBase(hostname, maslibs)}/lib/commerce.js`;
}

Check warning on line 388 in libs/blocks/merch/merch.js

View check run for this annotation

Codecov / codecov/patch

libs/blocks/merch/merch.js#L387-L388

Added lines #L387 - L388 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let commerceLibPath = '../../deps/commerce.js';
if (maslibs) {
commerceLibPath = `${getMasBase(hostname, maslibs)}/lib/commerce.js`;
}
const commerceLibPath = `${!maslibs ? '../../deps' : `${getMasBase(hostname, maslibs)}/lib`}/commerce.js`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it but I find it harder to read. Most of the time, maslibs will be false and the whole expression will be skipped.

const commerceLib = await import(commerceLibPath);
const service = await commerceLib.init(() => ({
env,
commerce,
Expand All @@ -376,8 +413,9 @@
}

/**
* Checkout parameter can be set Merch link, code config (scripts.js) or be a default from tacocat.
* To get the default, 'undefinded' should be passed, empty string will trigger an error!
* Checkout parameter can be set on the merch link,
* code config (scripts.js) or be a default from tacocat.
* To get the default, 'undefined' should be passed, empty string will trigger an error!
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a lint change? (fine with me, just clarifying)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also typo: undefinded :D

Copy link
Contributor

Choose a reason for hiding this comment

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

hehe

*
* clientId - code config -> default (adobe_com)
* workflow - merch link -> metadata -> default (UCv3)
Expand Down
4 changes: 4 additions & 0 deletions test/blocks/merch/mas/lib/commerce.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const mock = true;
const init = () => ({ imsSignedInPromise: Promise.resolve(), mock });
// eslint-disable-next-line import/prefer-default-export
export { init };
73 changes: 71 additions & 2 deletions test/blocks/merch/merch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import merch, {
getCheckoutAction,
PRICE_LITERALS_URL,
PRICE_TEMPLATE_REGULAR,
getMasBase,
} from '../../../libs/blocks/merch/merch.js';

import { mockFetch, unmockFetch, readMockText } from './mocks/fetch.js';
Expand Down Expand Up @@ -72,6 +73,16 @@ const config = {
placeholders: { 'upgrade-now': 'Upgrade Now', download: 'Download' },
};

const updateSearch = ({ maslibs } = {}) => {
const url = new URL(window.location);
if (!maslibs) {
url.searchParams.delete('maslibs');
} else {
url.searchParams.set('maslibs', maslibs);
}
window.history.pushState({}, '', url);
};

/**
* utility function that tests Price spans against mock HTML
*
Expand Down Expand Up @@ -147,6 +158,7 @@ describe('Merch Block', () => {

afterEach(() => {
setSubscriptionsData();
updateSearch();
});

it('does not decorate merch with bad content', async () => {
Expand Down Expand Up @@ -531,12 +543,22 @@ describe('Merch Block', () => {
});

it('getCheckoutAction: handles errors gracefully', async () => {
const action = await getCheckoutAction([{ productArrangement: {} }], {}, Promise.reject(new Error('error')));
expect(action).to.be.undefined;
const imsSignedInPromise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error('error'));
}, 1);
});
const action = await getCheckoutAction([{ productArrangement: {} }], {}, imsSignedInPromise);
expect(action).to.be.empty;
});
});

describe('Upgrade Flow', () => {
beforeEach(() => {
getMasBase.baseUrl = undefined;
updateSearch({});
});

it('updates CTA text to Upgrade Now', async () => {
mockIms();
getUserEntitlements();
Expand Down Expand Up @@ -668,4 +690,51 @@ describe('Merch Block', () => {
});
});
});

describe.skip('M@S consumption', () => {
describe('maslibs parameter', () => {
beforeEach(() => {
getMasBase.baseUrl = undefined;
updateSearch({});
});

it('should load commerce.js via maslibs', async () => {
initService.promise = undefined;
getMasBase.baseUrl = 'http://localhost:2000/test/blocks/merch/mas';
updateSearch({ maslibs: 'test' });
setConfig(config);
await mockIms();
const commerce = await initService(true);
expect(commerce.mock).to.be.true;
});

it('should return the default Adobe URL if no maslibs parameter is present', () => {
expect(getMasBase()).to.equal('https://www.adobe.com/mas');
});

it('should return the stage Adobe URL if maslibs=stage', () => {
expect(getMasBase('https://www.adobe.com', 'stage')).to.equal('https://www.stage.adobe.com/mas');
});

it('should return the local URL if maslibs=local', () => {
expect(getMasBase('https://www.adobe.com', 'local')).to.equal('http://localhost:8000');
});

it('should return the adobecom hlx live URL if maslibs is branch name', () => {
expect(getMasBase('https://main--milo--adobecom-hlx.live', 'test')).to.equal('https://test--mas--adobecom.hlx.live');
});

it('should return the hlx live URL from the fork if maslibs contains double dashes', () => {
expect(getMasBase('https://main--milo--adobecom-hlx.live', 'test--mas--user')).to.equal('https://test--mas--user.hlx.live');
});

it('should return the adobecom hlx page URL if maslibs is branch name', () => {
expect(getMasBase('https://main--milo--adobecom-hlx.page', 'test')).to.equal('https://test--mas--adobecom.hlx.page');
});

it('should return the hlx page URL from the fork if maslibs contains double dashes', () => {
expect(getMasBase('https://main--milo--adobecom-hlx.page', 'test--mas--user')).to.equal('https://test--mas--user.hlx.page');
});
});
});
});
Loading