[MWPW-161098] Links conversion localization support #3120
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #3120 +/- ##
=======================================
Coverage 96.34% 96.34%
=======================================
Files 245 245
Lines 56316 56322 +6
=======================================
+ Hits 54256 54264 +8
+ Misses 2060 2058 -2 ☔ View full report in Codecov by Sentry. |
| @@ -647,21 +647,27 @@ const decorateCopyLink = (a, evt) => { | |||
| }; | |||
|
|
|||
| export function convertStageLinks({ anchors, config, hostname, href }) { | |||
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That sounds a little confusing, would it not be already possible to add the locale part to the regex as an optional parameter? 🤔
There was a problem hiding this comment.
That was my initial thought, but you'll end up losing the locale part when doing the conversion with String.replace()
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
cc: @mokimo @narcis-radu @overmyheadandbody if you want to chime in too :)
There was a problem hiding this comment.
@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 😬
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
My initial feeling tells me, there should be a simpler solution, then again - I'm not the one who conducted the due diligence and found all the corner cases.
Thanks for being diligent and answering the questions and doing the education work and providing context!
|
Reminder to set the |
There was a problem hiding this comment.
Verified. Ready for Stage.
Testing details https://jira.corp.adobe.com/browse/MWPW-161098
|
Skipped 3120: "[MWPW-161098] Links conversion localization support" due to file "libs/utils/utils.js" overlap. Merging will be attempted in the next batch |
Description
This PR adds localization support for the links conversion feature.
Related Issue
Resolves: MWPW-161098
Testing instructions
Go on the test url and check if the conversions are done according to the
stageDomainsMapdefined inscripts.jsTest URLs
Acrobat:
Before: https://www.stage.adobe.com/acrobat.html
After: https://www.stage.adobe.com/acrobat.html?milolibs=mwpw-161098-loc-support--milo--robert-bogos
BACOM:
Before: https://main--bacom--adobecom.hlx.page/products/real-time-customer-data-platform/rtcdp
After: https://main--bacom--adobecom.hlx.page/products/real-time-customer-data-platform/rtcdp?milolibs=mwpw-161098-loc-support--milo--robert-bogos
CC:
Before: https://main--cc--adobecom.hlx.page/drafts/rbogos/default?martech=off
After: https://main--cc--adobecom.hlx.page/drafts/rbogos/default?martech=off&milolibs=mwpw-161098-loc-support--milo--robert-bogos
Homepage:
Before: https://main--homepage--adobecom.hlx.page/homepage/index-loggedout
After: https://main--homepage--adobecom.hlx.page/homepage/index-loggedout?milolibs=mwpw-161098-loc-support--milo--robert-bogos
Blog:
Before: https://main--blog--adobecom.hlx.page/
After: https://main--blog--adobecom.hlx.page/?milolibs=mwpw-161098-loc-support--milo--robert-bogos
Milo:
Before: https://main--milo--adobecom.hlx.page/ae_ar/drafts/rbogos/env-relative-urls?martech=off
After: https://mwpw-161098-loc-support--milo--robert-bogos.hlx.page/ae_ar/drafts/rbogos/env-relative-urls?martech=off