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-161098] Links conversion localization support #3120

Merged
merged 3 commits into from
Nov 7, 2024
Merged

[MWPW-161098] Links conversion localization support #3120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0405227
links conversion localization support
robert-bogos Oct 31, 2024
82b6da1
Merge branch 'stage' of https://github.com/adobecom/milo into mwpw-16…
robert-bogos Oct 31, 2024
7d8cd8e
added unit tests
robert-bogos Oct 31, 2024
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
2 changes: 1 addition & 1 deletion libs/scripts/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import locales from '../utils/locales.js';

// Production Domain
const prodDomains = ['milo.adobe.com'];
const prodDomains = ['milo.adobe.com', 'business.adobe.com', 'www.adobe.com'];

const stageDomainsMap = {
'www.stage.adobe.com': {
Expand Down
14 changes: 10 additions & 4 deletions libs/utils/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,21 +647,27 @@ const decorateCopyLink = (a, evt) => {
};

export function convertStageLinks({ anchors, config, hostname, href }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I lack the context, hence the question:
No matter if this is done before or after the localization, we'd just swap out the host, no? Since localization is part of the path - we aren't touching it and this should already be done somewhere from the milo links conversion 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this does look a bit more convoluted than needed, at least at a first glance. The localizeLink method should handle all the complexities of handling the locale prefix. There are use-cases we need to consider, such as #_dnt links, which should not be translated. That even has a originHostName parameter, which might come in handy when trying to switch up origins.

Copy link
Contributor Author

@robert-bogos robert-bogos Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These changes were necessary for use cases where different consumers share the same domain but are differentiated by path (e.g., bacom and bacom blog).

For example, without the updates in this PR, if a consumer sets the following mapping:

  '^https://some.consumer': {
    '^https://business.adobe.com(?!/blog)': 'https://business.stage.adobe.com',
    '^https://business.adobe.com/blog': 'https://main--bacom-blog--adobecom.hlx.page',
  },

they would expect that, on https://some.consumer, business.adobe.com would convert to business.stage.adobe.com and business.adobe.com/blog to main--bacom-blog--adobecom.hlx.page.

This works when localization isn’t involved, but on https://some.consumer/de, both business.adobe.com/de/blog and business.adobe.com/de would convert to business.stage.adobe.com/de, which is wrong.

My solution is to remove the localization from the path, do the conversions and then reattach the localization.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds a little confusing, would it not be already possible to add the locale part to the regex as an optional parameter? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial thought, but you'll end up losing the locale part when doing the conversion with String.replace()

Copy link
Contributor

@vhargrave vhargrave Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@robert-bogos I feel super bad for writing this now, but still curious what you think. What if we made this a config option instead? Every consumer can decide for themselves if they want their stage links converted with an option like
stageLinkTransformation: on

Then this could be opt in, you'd have way simpler code, and we'd be sending less bytes to consumers. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @mokimo @narcis-radu @overmyheadandbody if you want to chime in too :)

Copy link
Contributor

@mokimo mokimo Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@vhargrave this is already opt in, albeit the config is a bit more involved to get this running rather than just on. We had discussed things back and forth a lot, but settled on this and had a few iterations of it.

This feature is not as trivial as one might think, has a lot of corner cases and can vary from each consumer to the next 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for explaining and sending this discussion @mokimo 🙌
I didn't realize the requirements were so broad and that config option certainly won't cover it lol.

if (config.env?.name === 'prod' || !config.stageDomainsMap) return;
const matchedRules = Object.entries(config.stageDomainsMap)
const { env, stageDomainsMap, locale } = config;
if (env?.name === 'prod' || !stageDomainsMap) return;
const matchedRules = Object.entries(stageDomainsMap)
.find(([domain]) => (new RegExp(domain)).test(href));
if (!matchedRules) return;
const [, domainsMap] = matchedRules;
[...anchors].forEach((a) => {
const hasLocalePrefix = a.pathname.startsWith(locale.prefix);
const noLocaleLink = hasLocalePrefix ? a.href.replace(locale.prefix, '') : a.href;
const matchedDomain = Object.keys(domainsMap)
.find((domain) => (new RegExp(domain)).test(a.href));
.find((domain) => (new RegExp(domain)).test(noLocaleLink));
if (!matchedDomain) return;
a.href = a.href.replace(
const convertedLink = noLocaleLink.replace(
new RegExp(matchedDomain),
domainsMap[matchedDomain] === 'origin'
? `${matchedDomain.includes('https') ? 'https://' : ''}${hostname}`
: domainsMap[matchedDomain],
);
const convertedUrl = new URL(convertedLink);
convertedUrl.pathname = `${hasLocalePrefix ? locale.prefix : ''}${convertedUrl.pathname}`;
a.href = convertedUrl.toString();
if (/(\.page|\.live).*\.html(?=[?#]|$)/.test(a.href)) a.href = a.href.replace(/\.html(?=[?#]|$)/, '');
});
}
Expand Down
12 changes: 10 additions & 2 deletions test/utils/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,16 +537,18 @@ describe('Utils', () => {
it('should convert links when stageDomainsMap provided without regex', async () => {
const stageConfig = {
...config,
locale: { prefix: '/ae_ar' },
env: { name: 'stage' },
stageDomainsMap,
};

Object.entries(stageDomainsMap).forEach(([hostname, domainsMap]) => {
const anchors = Object.keys(domainsMap).map((d) => utils.createTag('a', { href: `https://${d}` }));
const localizedAnchors = Object.keys(domainsMap).map((d) => utils.createTag('a', { href: `https://${d}/ae_ar` }));
const externalAnchors = externalDomains.map((url) => utils.createTag('a', { href: url }));

utils.convertStageLinks({
anchors: [...anchors, ...externalAnchors],
anchors: [...anchors, ...localizedAnchors, ...externalAnchors],
config: stageConfig,
hostname,
href: `https://${hostname}`,
Expand All @@ -565,16 +567,22 @@ describe('Utils', () => {
const { hostname, map } = stageDomainsMapWRegex;
const stageConfigWRegex = {
...config,
locale: { prefix: '/de' },
env: { name: 'stage' },
stageDomainsMap: map,
};

Object.entries(map).forEach(([, domainsMap]) => {
const anchors = Object.keys(domainsMap).map((d) => utils.createTag('a', { href: d.replace('^', '') }));
const localizedAnchors = Object.keys(domainsMap).map((d) => {
const convertedUrl = new URL(d.replace('^', ''));
convertedUrl.pathname = `de/${convertedUrl.pathname}`;
return utils.createTag('a', { href: convertedUrl.toString() });
});
const externalAnchors = externalDomains.map((url) => utils.createTag('a', { href: url }));

utils.convertStageLinks({
anchors: [...anchors, ...externalAnchors],
anchors: [...anchors, ...localizedAnchors, ...externalAnchors],
config: stageConfigWRegex,
hostname,
href: `https://${hostname}`,
Expand Down
Loading