-
Notifications
You must be signed in to change notification settings - Fork 17
MWPW-189894: Lingo issue with placeholder fix #748
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
Changes from 11 commits
edb4791
9ef385d
4c1e9ff
fe6a34d
11c062f
7e76079
d489b47
78f560a
38e9894
2f67cd8
67e81a1
ada1bea
afaf786
bebe16c
5f890a1
9b6408f
31ea9b5
396dadb
f9cd222
82eef2b
92445f8
f0380ee
de3a795
c87863a
1cf2108
1132aee
70e6927
7d45741
9ea4692
0cb7389
cbeccca
5820296
702dcee
b6552a9
bf61073
b37a6ab
9edc248
2fe294a
5e3f989
0d78468
c52ea30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| import { odinReferences, odinUrl } from '../utils/paths.js'; | ||
| import { fetch, getFragmentId, getRequestInfos } from '../utils/common.js'; | ||
| import { getRequestInfos } from '../utils/common.js'; | ||
| import { log, logDebug } from '../utils/log.js'; | ||
| import { getDefaultLocaleCode, getLocaleCode, getRegionLocales, parseLocaleCode } from '../locales.js'; | ||
| import { computeRegionLocale, getDefaultLanguageVariation } from './fetchFragment.js'; | ||
|
|
||
| const PZN_FOLDER = '/pzn/'; | ||
|
|
||
|
|
@@ -13,53 +12,16 @@ function skimFragmentFromReferences(fragment) { | |
| return skimmedFragment; | ||
| } | ||
|
|
||
| /** | ||
| * get fragment associated to default language, just returning the body | ||
| * @param {*} context | ||
| * - 'locale' comes from request parameter, so can be optional | ||
| * - 'parsedLocale' is the actual location of the fetched fragment | ||
| * @returns null if something wrong, [] if not found, body if found | ||
| */ | ||
| async function getDefaultLanguageVariation(context) { | ||
| let { body } = context; | ||
| const { surface, locale, fragmentPath, preview, parsedLocale } = context; | ||
| // if no 'locale' request parameter, serve fragment as is | ||
| if (!locale) { | ||
| context.defaultLocale = parsedLocale; | ||
| return { body, parsedLocale, status: 200 }; | ||
| } | ||
| const defaultLocale = getDefaultLocaleCode(surface, locale); | ||
| if (!defaultLocale) { | ||
| return { status: 400, message: `Default locale not found for requested locale '${locale}'` }; | ||
| } | ||
| if (defaultLocale !== parsedLocale) { | ||
| logDebug(() => `Looking for fragment id for ${surface}/${defaultLocale}/${fragmentPath}`, context); | ||
| const defaultLocaleIdUrl = odinUrl(surface, { locale: defaultLocale, fragmentPath, preview }); | ||
| const { id: defaultLocaleId, status, message } = await getFragmentId(context, defaultLocaleIdUrl, 'default-locale-id'); | ||
| if (status != 200) { | ||
| return { status, message }; | ||
| } | ||
| const defaultLocaleUrl = odinReferences(defaultLocaleId, true, preview); | ||
| const response = await fetch(defaultLocaleUrl, context, 'default-locale-fragment'); | ||
| if (response.status != 200 || !response.body) { | ||
| /* c8 ignore next */ | ||
| const message = response.message || 'Error fetching default locale fragment'; | ||
| /* c8 ignore next */ | ||
| return { status: response.status || 503, message }; | ||
| } | ||
| ({ body } = response); | ||
| } | ||
| context.defaultLocale = defaultLocale; | ||
| return { body, defaultLocale, status: 200 }; | ||
| } | ||
|
|
||
| async function init(context) { | ||
| if (context?.promises?.fetchFragment) { | ||
| return await context.promises.fetchFragment; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is that i don't understand?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this is better for maintainability and clearer boundary between transformers but I guess I made you confused. I’ll update them to remove init() from customize.js. |
||
| const { body, surface, fragmentPath, parsedLocale } = await getRequestInfos(context); | ||
| context = { ...context, surface, fragmentPath, parsedLocale, body }; | ||
| const merged = { ...context, surface, fragmentPath, parsedLocale, body }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would not use "merge" here as it has special meaning in that transformer's context, and tbh at this stage you should already all all of those informations in the context (or if not please make fetchFragment process add it) |
||
| if (!surface || !fragmentPath) { | ||
| return { status: 400, message: 'Missing surface or fragmentPath' }; | ||
| } | ||
| return await getDefaultLanguageVariation(context); | ||
| return await getDefaultLanguageVariation(merged); | ||
| } | ||
|
|
||
| function deepMerge(...objects) { | ||
|
|
@@ -285,37 +247,6 @@ function customizeTree(root, referencesTree = [], customizeContext) { | |
| return { fragment: customizedRoot, references: customizeContext.references, referencesTree: adaptedTree }; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the locale used for regional paths and personalization. | ||
| * If the request uses the default locale code but country differs from that locale's default country and maps to a | ||
| * known region for that language on the surface, returns that regional code (e.g. fr_FR + CA → fr_CA). | ||
| * If the requested locale is already a regional code, it is preserved when no country override applies. | ||
| * @param {*} context | ||
| * @returns {string} | ||
| */ | ||
| export function computeRegionLocale(context) { | ||
| const { locale, defaultLocale: defaultLocaleCode, surface } = context; | ||
| const country = context.country?.toUpperCase(); | ||
| const [, defaultCountry] = parseLocaleCode(defaultLocaleCode); | ||
| const defaultCountryUpper = defaultCountry?.toUpperCase(); | ||
| const effectiveCountry = country && defaultCountryUpper != null && country !== defaultCountryUpper ? country : null; | ||
|
|
||
| let regionLocale = locale; | ||
| if (locale !== defaultLocaleCode || effectiveCountry != null) { | ||
| const regionObjects = getRegionLocales(surface, defaultLocaleCode, true); | ||
| const regionLocaleObject = | ||
| effectiveCountry != null ? regionObjects.find((r) => r.country?.toUpperCase() === effectiveCountry) : null; | ||
| const mapped = regionLocaleObject ? getLocaleCode(regionLocaleObject) : null; | ||
| regionLocale = mapped || locale; | ||
| } | ||
| logDebug( | ||
| () => | ||
| `Computed region locale '${regionLocale}' for requested locale '${locale}' with country '${country}' on surface '${surface}'`, | ||
| context, | ||
| ); | ||
| return regionLocale; | ||
| } | ||
|
|
||
| async function customize(context) { | ||
| const { surface } = await getRequestInfos(context); | ||
| const { body, defaultLocale, status, message } = await context.promises?.customize; | ||
|
|
@@ -326,7 +257,7 @@ async function customize(context) { | |
| } | ||
| const baseFragment = skimFragmentFromReferences(body); | ||
| const { references, referencesTree } = body; | ||
| const regionLocale = computeRegionLocale({ ...context, defaultLocale, surface }); | ||
| const regionLocale = context.regionLocale ?? computeRegionLocale({ ...context, defaultLocale, surface }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why would context.regionLocale be unset? we should always have it here, and no mention of computeRegionLocale at all
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed The Example: a test builds { locale: 'fr_FR', country: 'CA', body, surface, … } and expects the Canadian variation path — regionLocale has to come from the init-shaped result, not from context, because there is no prior fetchFragment step in the test harness. |
||
| const isRegionLocale = regionLocale !== defaultLocale; | ||
| const customizeContext = { | ||
| ...context, | ||
|
|
@@ -347,6 +278,8 @@ async function customize(context) { | |
| ...context, | ||
| status: 200, | ||
| body: customizedFragment, | ||
| locale: regionLocale, | ||
| defaultLocale, | ||
|
seanchoi-dev marked this conversation as resolved.
|
||
| }; | ||
| } | ||
|
|
||
|
|
@@ -356,3 +289,4 @@ export const transformer = { | |
| init, | ||
| }; | ||
| export { deepMerge }; | ||
| export { computeRegionLocale, getDefaultLanguageVariation } from './fetchFragment.js'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,12 +154,9 @@ function replaceValues(input, dictionary, calls) { | |
| } | ||
|
|
||
| async function init(context) { | ||
| // we fetch dictionary at this stage only if id has already been cached | ||
| // because we can't know surface of fragment *before* first fetch | ||
| // if dictionaryId is present in cache - early load dictionary | ||
| // if nothing in cache - dictionaryId and dictionary itself will be loaded later, | ||
| // during process | ||
| return await getDictionary(context); | ||
| const fetchResult = await context?.promises?.fetchFragment; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the end you still wait for both, no?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You’re right that I updated some code according to your suggestion, Here is an AI report on the performance changes in this branch: |
||
| const merged = fetchResult?.status === 200 ? { ...context, ...fetchResult } : context; | ||
| return await getDictionary(merged); | ||
| } | ||
|
|
||
| async function replace(context) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,14 @@ | ||
| import { getJsonFromState, mark, measureTiming } from './common.js'; | ||
| import { log } from './log.js'; | ||
| const getRequestMetadataKey = (context) => `req-${context.id}-${context.locale}`; | ||
| function getRequestMetadataKey(context) { | ||
| const id = context.id; | ||
| const locale = (context.requestLocale ?? context.locale) || 'no-locale'; | ||
| const raw = context.country; | ||
| if (raw != null && String(raw).trim() !== '') { | ||
| return `req-${id}-${locale}-${String(raw).trim().toUpperCase()}`; | ||
|
seanchoi-dev marked this conversation as resolved.
Outdated
|
||
| } | ||
| return `req-${id}-${locale}`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while agreeing it's a needed fix, i would make cache key format here, that is |
||
| } | ||
|
|
||
| async function getRequestMetadata(context) { | ||
| const requestKey = getRequestMetadataKey(context); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above