From 6aabe915e891bbbac5cc52658136d87ef21eddfd Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 21 Dec 2021 14:45:09 -0500 Subject: [PATCH 01/21] Added tasts --- .vscode/tasks.json | 70 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 .vscode/tasks.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json new file mode 100644 index 000000000000..ea550d21e7a9 --- /dev/null +++ b/.vscode/tasks.json @@ -0,0 +1,70 @@ +{ + "version": "2.0.0", + "tasks": [ + { + "label": "amp: Run unit test in this file", + "detail": "amp unit --files=${file} --headless --verbose", + "type": "shell", + "command": "amp", + "args": ["unit", "--files=${file}", "--headless", "--verbose"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + { + "label": "amp: Watch unit test in this file", + "detail": "amp unit --files=${file} --headless --verbose --watch", + "type": "shell", + "command": "amp", + "args": ["unit", "--files=${file}", "--headless", "--verbose", "--watch"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + }, + "isBackground": true + }, + { + "label": "amp: Run unit tests in all changed files", + "detail": "amp unit --local_changes --headless --verbose", + "type": "shell", + "command": "amp", + "args": ["unit", "--local_changes", "--headless", "--verbose"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + { + "label": "amp: Check PR", + "detail": "amp pr-check --nobuild", + "type": "shell", + "command": "amp", + "args": ["pr-check", "--nobuild"], + "group": "test", + "presentation": { + "echo": true, + "reveal": "always", + "focus": false, + "panel": "shared", + "showReuseMessage": true, + "clear": true + } + }, + ] +} \ No newline at end of file From dc1fa876f8e029a54a7cfd4bf2f70795da071102 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 21 Dec 2021 14:47:32 -0500 Subject: [PATCH 02/21] Undo --- .vscode/tasks.json | 70 ---------------------------------------------- 1 file changed, 70 deletions(-) delete mode 100644 .vscode/tasks.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json deleted file mode 100644 index ea550d21e7a9..000000000000 --- a/.vscode/tasks.json +++ /dev/null @@ -1,70 +0,0 @@ -{ - "version": "2.0.0", - "tasks": [ - { - "label": "amp: Run unit test in this file", - "detail": "amp unit --files=${file} --headless --verbose", - "type": "shell", - "command": "amp", - "args": ["unit", "--files=${file}", "--headless", "--verbose"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - { - "label": "amp: Watch unit test in this file", - "detail": "amp unit --files=${file} --headless --verbose --watch", - "type": "shell", - "command": "amp", - "args": ["unit", "--files=${file}", "--headless", "--verbose", "--watch"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - }, - "isBackground": true - }, - { - "label": "amp: Run unit tests in all changed files", - "detail": "amp unit --local_changes --headless --verbose", - "type": "shell", - "command": "amp", - "args": ["unit", "--local_changes", "--headless", "--verbose"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - { - "label": "amp: Check PR", - "detail": "amp pr-check --nobuild", - "type": "shell", - "command": "amp", - "args": ["pr-check", "--nobuild"], - "group": "test", - "presentation": { - "echo": true, - "reveal": "always", - "focus": false, - "panel": "shared", - "showReuseMessage": true, - "clear": true - } - }, - ] -} \ No newline at end of file From a04f6d2bea71e95042af7716250a2aa1d69bf9bb Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 8 Mar 2022 16:24:49 -0500 Subject: [PATCH 03/21] Added logic and tests --- examples/amp-story/inline_strings.html | 64 +++++++++++++++++++ extensions/amp-story/1.0/amp-story.js | 11 ++++ .../amp-story/1.0/test/test-amp-story.js | 50 ++++++++++++++- 3 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 examples/amp-story/inline_strings.html diff --git a/examples/amp-story/inline_strings.html b/examples/amp-story/inline_strings.html new file mode 100644 index 000000000000..61bba4881b60 --- /dev/null +++ b/examples/amp-story/inline_strings.html @@ -0,0 +1,64 @@ + + + + + Interactive Story + + + + + + + + + + + + + + + + + + +
+

The attachment should say "INLINED-STRING"

+ 
inlined strings:
{"35": "INLINED-STRING"}
+
+ + +

This is a page attachment

+ + + + + diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 074c7bb2701a..bfb32aa02339 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -344,6 +344,17 @@ export class AmpStory extends AMP.BaseElement { createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) ); + // If there are inline strings, register as current document language. + const inlineStrings = this.win.document.querySelector( + 'script[amp-strings="amp-story"]' + ); + if (inlineStrings) { + getLocalizationService(this.element).registerLocalizedStringBundle( + this.win.document.querySelector('[lang]')?.getAttribute('lang') || 'en', + JSON.parse(inlineStrings.textContent) + ); + } + if (this.isStandalone_()) { this.initializeStandaloneStory_(); } diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 6aee9fb09f99..39ff1b4156e2 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -49,6 +49,7 @@ describes.realWin( let story; let replaceStateStub; let win; + let localizationService; const nextTick = () => new Promise((resolve) => win.setTimeout(resolve, 0)); @@ -98,7 +99,7 @@ describes.realWin( replaceStateStub = env.sandbox.stub(win.history, 'replaceState'); - const localizationService = new LocalizationService(win.document.body); + localizationService = new LocalizationService(win.document.body); env.sandbox .stub(Services, 'localizationForDoc') .returns(localizationService); @@ -1624,5 +1625,52 @@ describes.realWin( ); }); }); + + describe.only('localization', () => { + it('should have the default localizations', async () => { + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'Swipe up' + ); + }); + + it('should use the correct language localizations', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'es'); + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'Deslizar el dedo hacia arriba' + ); + }); + + it('should use the inlined amp-story strings when available', async () => { + const inlinedStrings = win.document.createElement('script'); + inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.textContent = '{"35": "INLINED-STRING"}'; + win.document.head.appendChild(inlinedStrings); + + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'INLINED-STRING' + ); + }); + + it('should use the inlined amp-story strings when available if the language is specified', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'es'); + + const inlinedStrings = win.document.createElement('script'); + inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.textContent = '{"35": "TEXTO-EN-LINEA"}'; + win.document.head.appendChild(inlinedStrings); + + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'TEXTO-EN-LINEA' + ); + }); + }); } ); From 9c4f9b9b28a2982641a7b06cec71c2f9cc36db12 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 8 Mar 2022 17:17:22 -0500 Subject: [PATCH 04/21] Updated tests with correct version --- examples/amp-story/inline_strings.html | 8 +++++- extensions/amp-story/1.0/amp-story.js | 8 +++++- .../amp-story/1.0/test/test-amp-story.js | 28 +++++++++++++++++-- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/examples/amp-story/inline_strings.html b/examples/amp-story/inline_strings.html index 61bba4881b60..944d87822c9e 100644 --- a/examples/amp-story/inline_strings.html +++ b/examples/amp-story/inline_strings.html @@ -27,11 +27,17 @@ justify-content: center; } + - + { - it('should have the default localizations', async () => { + describe('localization', () => { + beforeEach(() => { + win.__AMP_MODE = { + rtvVersion: '123', + }; + }); + + it('should install the default english localizations', async () => { await createStoryWithPages(1, ['cover']); expect(localizationService.getLocalizedString('35')).to.be.equal( @@ -1635,7 +1641,7 @@ describes.realWin( ); }); - it('should use the correct language localizations', async () => { + it('should install the correct language localizations if specified', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es'); await createStoryWithPages(1, ['cover']); @@ -1647,6 +1653,7 @@ describes.realWin( it('should use the inlined amp-story strings when available', async () => { const inlinedStrings = win.document.createElement('script'); inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('i-amphtml-version', '123'); inlinedStrings.textContent = '{"35": "INLINED-STRING"}'; win.document.head.appendChild(inlinedStrings); @@ -1657,11 +1664,26 @@ describes.realWin( ); }); + it('should not use the inlined amp-story strings if incorrect RTV', async () => { + const inlinedStrings = win.document.createElement('script'); + inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('i-amphtml-version', '1234'); + inlinedStrings.textContent = '{"35": "INLINED-STRING"}'; + win.document.head.appendChild(inlinedStrings); + + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'Swipe up' + ); + }); + it('should use the inlined amp-story strings when available if the language is specified', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es'); const inlinedStrings = win.document.createElement('script'); inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('i-amphtml-version', '123'); inlinedStrings.textContent = '{"35": "TEXTO-EN-LINEA"}'; win.document.head.appendChild(inlinedStrings); From 07e3ca5c8cf1cb193642a72e7bed23c0d1c09dc5 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 12:14:43 -0300 Subject: [PATCH 05/21] Updated comment Co-authored-by: Gabriel Majoulet --- extensions/amp-story/1.0/amp-story.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 13f9fd3a1dae..dc4d9e765c16 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -346,7 +346,7 @@ export class AmpStory extends AMP.BaseElement { createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) ); - // If there are inline strings, register as current document language. + // If there are inline localization strings, register as current document language. const inlineStrings = this.win.document.querySelector( 'script[amp-strings="amp-story"]' ); From 879fa46865d55367f46984cea09cd5488fa9182c Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 10:23:05 -0500 Subject: [PATCH 06/21] Catch error in JSON inlined --- extensions/amp-story/1.0/amp-story.js | 45 ++++++++++++------- .../amp-story/1.0/test/test-amp-story.js | 18 +++++++- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 13f9fd3a1dae..1a4afc896ca5 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -42,10 +42,10 @@ import { toggle, } from '#core/dom/style'; import {isEsm} from '#core/mode'; -import {version} from '#core/mode/version'; import {findIndex, lastItem, toArray} from '#core/types/array'; import {debounce} from '#core/types/function'; import {map} from '#core/types/object'; +import {tryParseJson} from '#core/types/object/json'; import {endsWith} from '#core/types/string'; import {parseQueryString} from '#core/types/string/url'; import {getHistoryState as getWindowHistoryState} from '#core/window/history'; @@ -345,21 +345,7 @@ export class AmpStory extends AMP.BaseElement { 'en-xa', createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) ); - - // If there are inline strings, register as current document language. - const inlineStrings = this.win.document.querySelector( - 'script[amp-strings="amp-story"]' - ); - if ( - inlineStrings && - inlineStrings.getAttribute('i-amphtml-version') == - getMode(this.win).rtvVersion - ) { - getLocalizationService(this.element).registerLocalizedStringBundle( - this.win.document.querySelector('[lang]')?.getAttribute('lang') || 'en', - JSON.parse(inlineStrings.textContent) - ); - } + this.registerInlineLocalizationStrings_(); if (this.isStandalone_()) { this.initializeStandaloneStory_(); @@ -2484,6 +2470,33 @@ export class AmpStory extends AMP.BaseElement { extensionsFor.installExtensionForDoc(ampdoc, 'amp-analytics'); } } + + /** + * If there are inline strings, register as current document language. + */ + registerInlineLocalizationStrings_() { + const inlineStringsEl = this.win.document.querySelector( + 'script[amp-strings="amp-story"]' + ); + if ( + !inlineStringsEl || + inlineStringsEl.getAttribute('i-amphtml-version') != + getMode(this.win).rtvVersion + ) { + return; + } + + const stringsOrNull = tryParseJson(inlineStringsEl.textContent); + + if (!stringsOrNull) { + return; + } + + getLocalizationService(this.element).registerLocalizedStringBundle( + this.win.document.querySelector('[lang]')?.getAttribute('lang') || 'en', + stringsOrNull + ); + } } AMP.extension('amp-story', '1.0', (AMP) => { diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index e6947d80008b..23a98a1cfa62 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1626,7 +1626,7 @@ describes.realWin( }); }); - describe('localization', () => { + describe.only('localization', () => { beforeEach(() => { win.__AMP_MODE = { rtvVersion: '123', @@ -1693,6 +1693,22 @@ describes.realWin( 'TEXTO-EN-LINEA' ); }); + + it('should use the default strings if inlined JSON is corrupted', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'en'); + + const inlinedStrings = win.document.createElement('script'); + inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('i-amphtml-version', '123'); + inlinedStrings.textContent = 'this: is not a JSON'; + win.document.head.appendChild(inlinedStrings); + + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'Swipe up' + ); + }); }); } ); From bc9c7ed779fb8b9d3021e6336a4583999e46fc41 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 10:31:24 -0500 Subject: [PATCH 07/21] Removed empty lines --- extensions/amp-story/1.0/amp-story.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index db394cf51fb0..1610bcac931c 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -2485,13 +2485,10 @@ export class AmpStory extends AMP.BaseElement { ) { return; } - const stringsOrNull = tryParseJson(inlineStringsEl.textContent); - if (!stringsOrNull) { return; } - getLocalizationService(this.element).registerLocalizedStringBundle( this.win.document.querySelector('[lang]')?.getAttribute('lang') || 'en', stringsOrNull From e53661ac8f3f314199670eb7ed57438f670c8d8b Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 10:55:28 -0500 Subject: [PATCH 08/21] Changed attribute to amp-localization --- examples/amp-story/inline_strings.html | 2 +- extensions/amp-story/1.0/amp-story.js | 2 +- extensions/amp-story/1.0/test/test-amp-story.js | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/examples/amp-story/inline_strings.html b/examples/amp-story/inline_strings.html index 944d87822c9e..d3b435aefde0 100644 --- a/examples/amp-story/inline_strings.html +++ b/examples/amp-story/inline_strings.html @@ -37,7 +37,7 @@ - + { const inlinedStrings = win.document.createElement('script'); - inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('amp-localization', 'amp-story'); inlinedStrings.setAttribute('i-amphtml-version', '123'); inlinedStrings.textContent = '{"35": "INLINED-STRING"}'; win.document.head.appendChild(inlinedStrings); @@ -1666,7 +1666,7 @@ describes.realWin( it('should not use the inlined amp-story strings if incorrect RTV', async () => { const inlinedStrings = win.document.createElement('script'); - inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('amp-localization', 'amp-story'); inlinedStrings.setAttribute('i-amphtml-version', '1234'); inlinedStrings.textContent = '{"35": "INLINED-STRING"}'; win.document.head.appendChild(inlinedStrings); @@ -1682,7 +1682,7 @@ describes.realWin( env.win.document.body.parentElement.setAttribute('lang', 'es'); const inlinedStrings = win.document.createElement('script'); - inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('amp-localization', 'amp-story'); inlinedStrings.setAttribute('i-amphtml-version', '123'); inlinedStrings.textContent = '{"35": "TEXTO-EN-LINEA"}'; win.document.head.appendChild(inlinedStrings); @@ -1698,7 +1698,7 @@ describes.realWin( env.win.document.body.parentElement.setAttribute('lang', 'en'); const inlinedStrings = win.document.createElement('script'); - inlinedStrings.setAttribute('amp-strings', 'amp-story'); + inlinedStrings.setAttribute('amp-localization', 'amp-story'); inlinedStrings.setAttribute('i-amphtml-version', '123'); inlinedStrings.textContent = 'this: is not a JSON'; win.document.head.appendChild(inlinedStrings); From 55274f6d4e5d7d126f608edb2e8f26eee627aec0 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 12:22:41 -0500 Subject: [PATCH 09/21] Added json fetching for testing --- examples/amp-story/inline_strings.html | 2 +- extensions/amp-story/1.0/amp-story.js | 42 ++++++++++-- src/service/localization/index.js | 92 +++++++++++++++++++++++++- 3 files changed, 129 insertions(+), 7 deletions(-) diff --git a/examples/amp-story/inline_strings.html b/examples/amp-story/inline_strings.html index d3b435aefde0..a1226b74d50d 100644 --- a/examples/amp-story/inline_strings.html +++ b/examples/amp-story/inline_strings.html @@ -1,5 +1,5 @@ - + Interactive Story diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 91c040fc300d..bd00f641a10c 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -53,6 +53,10 @@ import {getHistoryState as getWindowHistoryState} from '#core/window/history'; import {isExperimentOn} from '#experiments'; import {Services} from '#service'; +import { + getLanguageCodesFromString, + getValidLanguageCodeFromList, +} from '#service/localization'; import {createPseudoLocale} from '#service/localization/strings'; import {getDetail} from '#utils/event-helper'; @@ -345,7 +349,9 @@ export class AmpStory extends AMP.BaseElement { 'en-xa', createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) ); - this.registerInlineLocalizationStrings_(); + if (!this.registerInlineLocalizationStrings_()) { + this.fetchLocalizationStrings_(); + } if (this.isStandalone_()) { this.initializeStandaloneStory_(); @@ -2473,6 +2479,7 @@ export class AmpStory extends AMP.BaseElement { /** * If there are inline localization strings, register as current document language. + * @return {boolean} */ registerInlineLocalizationStrings_() { const inlineStringsEl = this.win.document.querySelector( @@ -2483,16 +2490,43 @@ export class AmpStory extends AMP.BaseElement { inlineStringsEl.getAttribute('i-amphtml-version') != getMode(this.win).rtvVersion ) { - return; + return false; } const stringsOrNull = tryParseJson(inlineStringsEl.textContent); + const docLang = this.win.document + .querySelector('[lang]') + ?.getAttribute('lang'); + if (!stringsOrNull) { - return; + return false; } getLocalizationService(this.element).registerLocalizedStringBundle( - this.win.document.querySelector('[lang]')?.getAttribute('lang') || 'en', + docLang, stringsOrNull ); + return true; + } + + /** + * Fetches from the CDN or locally the localization strings. + */ + fetchLocalizationStrings_() { + const docLang = this.win.document + .querySelector('[lang]') + ?.getAttribute('lang'); + const langCodes = getLanguageCodesFromString(docLang); + const validLanguageCode = getValidLanguageCodeFromList(langCodes); + Services.xhrFor(this.win) + .fetchJson( + 'https://cdn.ampproject.org/v0/amp-story.' + validLanguageCode + '.json' + ) + .then((res) => res.json()) + .then((json) => + getLocalizationService(this.element).registerLocalizedStringBundle( + docLang, + json + ) + ); } } diff --git a/src/service/localization/index.js b/src/service/localization/index.js index 33c06b826823..2d7e138cecf8 100644 --- a/src/service/localization/index.js +++ b/src/service/localization/index.js @@ -15,7 +15,7 @@ import { * Language code used if there is no language code specified by the document. * @const {string} */ -const FALLBACK_LANGUAGE_CODE = 'default'; +const FALLBACK_LANGUAGE_CODE = 'en'; /** * @const {!RegExp} @@ -53,7 +53,7 @@ function findLocalizedString( */ export function getLanguageCodesFromString(languageCode) { if (!languageCode) { - return ['en', FALLBACK_LANGUAGE_CODE]; + return [FALLBACK_LANGUAGE_CODE]; } const matches = languageCode.match(LANGUAGE_CODE_CHUNK_REGEX) || []; return matches.reduce( @@ -69,6 +69,94 @@ export function getLanguageCodesFromString(languageCode) { ); } +/** + * Finds the first valid language from a list of language codes + * @param {string[]} codesList + * @return {string} + */ +export function getValidLanguageCodeFromList(codesList) { + // List generated from running in Python + // [x.replace(".json", "") for x in os.listdir("./extensions/amp-story/1.0/_locales") if ".json" in x] + const allLanguages = [ + 'tr', + 'mk', + 'sl', + 'hu', + 'mr', + 'lt', + 'is', + 'bn', + 'nl', + 'ms', + 'ja', + 'de', + 'ru', + 'pl', + 'uk', + 'fi', + 'ta', + 'fil', + 'ur', + 'zh-cn', + 'sk', + 'ml', + 'en', + 'ka', + 'pa', + 'my', + 'pt-pt', + 'km', + 'it', + 'sr', + 'hr', + 'es-419', + 'zu', + 'et', + 'iw', + 'kn', + 'sq', + 'ne', + 'bs', + 'fr', + 'am', + 'gu', + 'el', + 'bg', + 'ro', + 'hi', + 'ca', + 'mn', + 'si', + 'pt-br', + 'ko', + 'eu', + 'gl', + 'zh-tw', + 'vi', + 'fa', + 'lo', + 'cs', + 'te', + 'id', + 'lv', + 'no', + 'af', + 'sw', + 'da', + 'th', + 'en-gb', + 'sv', + 'es', + 'ar', + ]; + for (let i = 0; i < codesList.length; i++) { + if (allLanguages.includes(codesList[i])) { + return codesList[i]; + } + } + return FALLBACK_LANGUAGE_CODE; +} + /** * Localization service. */ From 595c683a4be951ac7efdefaf350f48123cc9ea5b Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Wed, 9 Mar 2022 15:59:38 -0500 Subject: [PATCH 10/21] Remove .only --- extensions/amp-story/1.0/test/test-amp-story.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index eb33aa65a88a..1a2d4797e6f2 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1626,7 +1626,7 @@ describes.realWin( }); }); - describe.only('localization', () => { + describe('localization', () => { beforeEach(() => { win.__AMP_MODE = { rtvVersion: '123', From 5b923de423e45af6d93d05b71b55359dea268743 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 12:26:21 -0500 Subject: [PATCH 11/21] Install localization strings if inside experiment --- extensions/amp-story/1.0/amp-story.js | 85 ++++++++++++++++----------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index bd00f641a10c..5893ff5a875b 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -53,6 +53,7 @@ import {getHistoryState as getWindowHistoryState} from '#core/window/history'; import {isExperimentOn} from '#experiments'; import {Services} from '#service'; +import {calculateScriptBaseUrl} from '#service/extension-script'; import { getLanguageCodesFromString, getValidLanguageCodeFromList, @@ -126,6 +127,7 @@ import { import {AnalyticsVariable, getVariableService} from './variable-service'; import {CSS} from '../../../build/amp-story-1.0.css'; +import {urls} from '../../../src/config'; import {getConsentPolicyState} from '../../../src/consent'; import {Gestures} from '../../../src/gesture'; import {SwipeXYRecognizer} from '../../../src/gesture-recognizers'; @@ -322,36 +324,7 @@ export class AmpStory extends AMP.BaseElement { ? new AmpStoryViewerMessagingHandler(this.win, this.viewer_) : null; - getLocalizationService(this.element) - .registerLocalizedStringBundle('default', LocalizedStringsEn) - .registerLocalizedStringBundle('ar', LocalizedStringsAr) - .registerLocalizedStringBundle('de', LocalizedStringsDe) - .registerLocalizedStringBundle('en', LocalizedStringsEn) - .registerLocalizedStringBundle('en-GB', LocalizedStringsEnGb) - .registerLocalizedStringBundle('es', LocalizedStringsEs) - .registerLocalizedStringBundle('es-419', LocalizedStringsEs419) - .registerLocalizedStringBundle('fr', LocalizedStringsFr) - .registerLocalizedStringBundle('hi', LocalizedStringsHi) - .registerLocalizedStringBundle('id', LocalizedStringsId) - .registerLocalizedStringBundle('it', LocalizedStringsIt) - .registerLocalizedStringBundle('ja', LocalizedStringsJa) - .registerLocalizedStringBundle('ko', LocalizedStringsKo) - .registerLocalizedStringBundle('nl', LocalizedStringsNl) - .registerLocalizedStringBundle('no', LocalizedStringsNo) - .registerLocalizedStringBundle('pt-PT', LocalizedStringsPtPt) - .registerLocalizedStringBundle('pt-BR', LocalizedStringsPtBr) - .registerLocalizedStringBundle('ru', LocalizedStringsRu) - .registerLocalizedStringBundle('tr', LocalizedStringsTr) - .registerLocalizedStringBundle('vi', LocalizedStringsVi) - .registerLocalizedStringBundle('zh-CN', LocalizedStringsZhCn) - .registerLocalizedStringBundle('zh-TW', LocalizedStringsZhTw) - .registerLocalizedStringBundle( - 'en-xa', - createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) - ); - if (!this.registerInlineLocalizationStrings_()) { - this.fetchLocalizationStrings_(); - } + this.installLocalizationStrings_(); if (this.isStandalone_()) { this.initializeStandaloneStory_(); @@ -2477,6 +2450,46 @@ export class AmpStory extends AMP.BaseElement { } } + /** + * Adds the localization string bundles to the localization service. + */ + installLocalizationStrings_() { + if (this.registerInlineLocalizationStrings_()) { + return; + } + if (isExperimentOn(this.win, 'story-remote-locales')) { + this.fetchLocalizationStrings_(); + } else { + getLocalizationService(this.element) + .registerLocalizedStringBundle('default', LocalizedStringsEn) + .registerLocalizedStringBundle('ar', LocalizedStringsAr) + .registerLocalizedStringBundle('de', LocalizedStringsDe) + .registerLocalizedStringBundle('en', LocalizedStringsEn) + .registerLocalizedStringBundle('en-GB', LocalizedStringsEnGb) + .registerLocalizedStringBundle('es', LocalizedStringsEs) + .registerLocalizedStringBundle('es-419', LocalizedStringsEs419) + .registerLocalizedStringBundle('fr', LocalizedStringsFr) + .registerLocalizedStringBundle('hi', LocalizedStringsHi) + .registerLocalizedStringBundle('id', LocalizedStringsId) + .registerLocalizedStringBundle('it', LocalizedStringsIt) + .registerLocalizedStringBundle('ja', LocalizedStringsJa) + .registerLocalizedStringBundle('ko', LocalizedStringsKo) + .registerLocalizedStringBundle('nl', LocalizedStringsNl) + .registerLocalizedStringBundle('no', LocalizedStringsNo) + .registerLocalizedStringBundle('pt-PT', LocalizedStringsPtPt) + .registerLocalizedStringBundle('pt-BR', LocalizedStringsPtBr) + .registerLocalizedStringBundle('ru', LocalizedStringsRu) + .registerLocalizedStringBundle('tr', LocalizedStringsTr) + .registerLocalizedStringBundle('vi', LocalizedStringsVi) + .registerLocalizedStringBundle('zh-CN', LocalizedStringsZhCn) + .registerLocalizedStringBundle('zh-TW', LocalizedStringsZhTw) + .registerLocalizedStringBundle( + 'en-xa', + createPseudoLocale(LocalizedStringsEn, (s) => `[${s} one two]`) + ); + } + } + /** * If there are inline localization strings, register as current document language. * @return {boolean} @@ -2516,10 +2529,16 @@ export class AmpStory extends AMP.BaseElement { ?.getAttribute('lang'); const langCodes = getLanguageCodesFromString(docLang); const validLanguageCode = getValidLanguageCodeFromList(langCodes); + + const localizationUrl = `${calculateScriptBaseUrl( + this.win.location, + getMode(this.win).localDev + )}/v0/amp-story.${validLanguageCode}.json`; + + console.log(urls); + Services.xhrFor(this.win) - .fetchJson( - 'https://cdn.ampproject.org/v0/amp-story.' + validLanguageCode + '.json' - ) + .fetchJson(localizationUrl) .then((res) => res.json()) .then((json) => getLocalizationService(this.element).registerLocalizedStringBundle( From 1d419c36ef7f6b291500c4d219ae3251829d8203 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 15:32:09 -0500 Subject: [PATCH 12/21] Added tests --- extensions/amp-story/1.0/amp-story.js | 34 ++++----- .../amp-story/1.0/test/test-amp-story.js | 71 +++++++++++++++++++ 2 files changed, 88 insertions(+), 17 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 7f3f3f4d7d55..b42e14351960 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -2454,11 +2454,15 @@ export class AmpStory extends AMP.BaseElement { * Adds the localization string bundles to the localization service. */ installLocalizationStrings_() { - if (this.registerInlineLocalizationStrings_()) { + const localizationService = getLocalizationService(this.element); + const storyLanguage = localizationService.getLanguageCodesForElement( + this.element + )[0]; + if (this.registerInlineLocalizationStrings_(storyLanguage)) { return; } if (isExperimentOn(this.win, 'story-remote-locales')) { - this.fetchLocalizationStrings_(); + this.fetchLocalizationStrings_(storyLanguage); } else { getLocalizationService(this.element) .registerLocalizedStringBundle('default', LocalizedStringsEn) @@ -2492,29 +2496,27 @@ export class AmpStory extends AMP.BaseElement { /** * If there are inline localization strings, register as current document language. + * @param {string} languageCode * @return {boolean} */ - registerInlineLocalizationStrings_() { + registerInlineLocalizationStrings_(languageCode) { const inlineStringsEl = this.win.document.querySelector( 'script[amp-localization="amp-story"]' ); if ( - !inlineStringsEl || - inlineStringsEl.getAttribute('i-amphtml-version') != - getMode(this.win).rtvVersion + inlineStringsEl?.getAttribute('i-amphtml-version') !== + getMode(this.win).rtvVersion ) { return false; } const stringsOrNull = tryParseJson(inlineStringsEl.textContent); - const docLang = this.win.document - .querySelector('[lang]') - ?.getAttribute('lang'); if (!stringsOrNull) { return false; } - getLocalizationService(this.element).registerLocalizedStringBundle( - docLang, + const localizationService = getLocalizationService(this.element); + localizationService.registerLocalizedStringBundle( + languageCode, stringsOrNull ); return true; @@ -2522,23 +2524,21 @@ export class AmpStory extends AMP.BaseElement { /** * Fetches from the CDN or localhost the localization strings. + * @param {string} languageCode */ - fetchLocalizationStrings_() { + fetchLocalizationStrings_(languageCode) { const localizationService = getLocalizationService(this.element); - const lang = localizationService.getLanguageCodesForElement( - this.element - )[0]; const localizationUrl = `${calculateScriptBaseUrl( this.win.location, getMode(this.win).localDev - )}/v0/amp-story.${lang}.json`; + )}/v0/amp-story.${languageCode}.json`; Services.xhrFor(this.win) .fetchJson(localizationUrl) .then((res) => res.json()) .then((json) => - localizationService.registerLocalizedStringBundle(lang, json) + localizationService.registerLocalizedStringBundle(languageCode, json) ); } } diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 1a2d4797e6f2..307b904f62ca 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1709,6 +1709,77 @@ describes.realWin( 'Swipe up' ); }); + + it('should fetch the localization strings for the default laguage from the cdn', async () => { + toggleExperiment(env.win, 'story-remote-locales'); + + const fetchSpy = env.sandbox.spy(Services.xhrFor(env.win), 'fetchJson'); + + await createStoryWithPages(1, ['cover']); + + expect(fetchSpy).to.be.calledOnceWithExactly( + 'https://cdn.ampproject.org/v0/amp-story.en.json' + ); + }); + + describe('remote locales', () => { + beforeEach(() => { + toggleExperiment(env.win, 'story-remote-locales', true); + }); + + it('should fetch the localization strings for the document laguage from the cdn', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'es-419'); + + const fetchSpy = env.sandbox.spy( + Services.xhrFor(env.win), + 'fetchJson' + ); + + await createStoryWithPages(1, ['cover']); + + expect(fetchSpy).to.have.been.calledWith( + 'https://cdn.ampproject.org/v0/amp-story.es-419.json' + ); + }); + + it('should fetch the localization strings for the document laguage from the local dist if testing locally', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'es-419'); + env.win.__AMP_MODE.localDev = true; + + const fetchSpy = env.sandbox.spy( + Services.xhrFor(env.win), + 'fetchJson' + ); + + await createStoryWithPages(1, ['cover']); + + expect(fetchSpy).to.have.been.calledWith( + '/dist/v0/amp-story.es-419.json' + ); + }); + + it('should use the remote localization strings', async () => { + env.win.document.body.parentElement.setAttribute('lang', 'es-419'); + + const fetchMock = env.sandbox.mock(Services.xhrFor(env.win)); + fetchMock.expects('fetchJson').returns( + Promise.resolve({ + json: () => { + return Promise.resolve({ + '35': 'REMOTE-STRING', + }); + }, + }) + ); + + await createStoryWithPages(1, ['cover']); + + expect(localizationService.getLocalizedString('35')).to.be.equal( + 'REMOTE-STRING' + ); + fetchMock.verify(); + }); + }); }); } ); From 72070cf853ddb89c2e797829a936d60eb777ad88 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 15:58:15 -0500 Subject: [PATCH 13/21] Moved test to inside describes --- .../amp-story/1.0/test/test-amp-story.js | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 307b904f62ca..677530000a00 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1710,23 +1710,24 @@ describes.realWin( ); }); - it('should fetch the localization strings for the default laguage from the cdn', async () => { - toggleExperiment(env.win, 'story-remote-locales'); - - const fetchSpy = env.sandbox.spy(Services.xhrFor(env.win), 'fetchJson'); - - await createStoryWithPages(1, ['cover']); - - expect(fetchSpy).to.be.calledOnceWithExactly( - 'https://cdn.ampproject.org/v0/amp-story.en.json' - ); - }); - describe('remote locales', () => { beforeEach(() => { toggleExperiment(env.win, 'story-remote-locales', true); }); + it('should fetch the localization strings for the default laguage from the cdn', async () => { + const fetchSpy = env.sandbox.spy( + Services.xhrFor(env.win), + 'fetchJson' + ); + + await createStoryWithPages(1, ['cover']); + + expect(fetchSpy).to.be.calledOnceWithExactly( + 'https://cdn.ampproject.org/v0/amp-story.en.json' + ); + }); + it('should fetch the localization strings for the document laguage from the cdn', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es-419'); From 1361273033f1bb3a221f41a84e47d112eb5b3d75 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 16:12:28 -0500 Subject: [PATCH 14/21] Remove unused function --- extensions/amp-story/1.0/amp-story.js | 4 -- src/service/localization/index.js | 88 --------------------------- 2 files changed, 92 deletions(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index b42e14351960..7e60442d1b08 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -54,10 +54,6 @@ import {isExperimentOn} from '#experiments'; import {Services} from '#service'; import {calculateScriptBaseUrl} from '#service/extension-script'; -import { - getLanguageCodesFromString, - getValidLanguageCodeFromList, -} from '#service/localization'; import {createPseudoLocale} from '#service/localization/strings'; import {getDetail} from '#utils/event-helper'; diff --git a/src/service/localization/index.js b/src/service/localization/index.js index 2d7e138cecf8..f3b1c67b0b4b 100644 --- a/src/service/localization/index.js +++ b/src/service/localization/index.js @@ -69,94 +69,6 @@ export function getLanguageCodesFromString(languageCode) { ); } -/** - * Finds the first valid language from a list of language codes - * @param {string[]} codesList - * @return {string} - */ -export function getValidLanguageCodeFromList(codesList) { - // List generated from running in Python - // [x.replace(".json", "") for x in os.listdir("./extensions/amp-story/1.0/_locales") if ".json" in x] - const allLanguages = [ - 'tr', - 'mk', - 'sl', - 'hu', - 'mr', - 'lt', - 'is', - 'bn', - 'nl', - 'ms', - 'ja', - 'de', - 'ru', - 'pl', - 'uk', - 'fi', - 'ta', - 'fil', - 'ur', - 'zh-cn', - 'sk', - 'ml', - 'en', - 'ka', - 'pa', - 'my', - 'pt-pt', - 'km', - 'it', - 'sr', - 'hr', - 'es-419', - 'zu', - 'et', - 'iw', - 'kn', - 'sq', - 'ne', - 'bs', - 'fr', - 'am', - 'gu', - 'el', - 'bg', - 'ro', - 'hi', - 'ca', - 'mn', - 'si', - 'pt-br', - 'ko', - 'eu', - 'gl', - 'zh-tw', - 'vi', - 'fa', - 'lo', - 'cs', - 'te', - 'id', - 'lv', - 'no', - 'af', - 'sw', - 'da', - 'th', - 'en-gb', - 'sv', - 'es', - 'ar', - ]; - for (let i = 0; i < codesList.length; i++) { - if (allLanguages.includes(codesList[i])) { - return codesList[i]; - } - } - return FALLBACK_LANGUAGE_CODE; -} - /** * Localization service. */ From 5b49ffffabb8c7f1416ab0b049952921acd403dd Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 16:17:50 -0500 Subject: [PATCH 15/21] Remove unused import --- extensions/amp-story/1.0/amp-story.js | 1 - 1 file changed, 1 deletion(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 7e60442d1b08..f7a87285435d 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -123,7 +123,6 @@ import { import {AnalyticsVariable, getVariableService} from './variable-service'; import {CSS} from '../../../build/amp-story-1.0.css'; -import {urls} from '../../../src/config'; import {getConsentPolicyState} from '../../../src/consent'; import {Gestures} from '../../../src/gesture'; import {SwipeXYRecognizer} from '../../../src/gesture-recognizers'; From b293243098eb1531a29c7329675c10d2402b2829 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 16:19:39 -0500 Subject: [PATCH 16/21] Reverted index.js --- src/service/localization/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/service/localization/index.js b/src/service/localization/index.js index f3b1c67b0b4b..33c06b826823 100644 --- a/src/service/localization/index.js +++ b/src/service/localization/index.js @@ -15,7 +15,7 @@ import { * Language code used if there is no language code specified by the document. * @const {string} */ -const FALLBACK_LANGUAGE_CODE = 'en'; +const FALLBACK_LANGUAGE_CODE = 'default'; /** * @const {!RegExp} @@ -53,7 +53,7 @@ function findLocalizedString( */ export function getLanguageCodesFromString(languageCode) { if (!languageCode) { - return [FALLBACK_LANGUAGE_CODE]; + return ['en', FALLBACK_LANGUAGE_CODE]; } const matches = languageCode.match(LANGUAGE_CODE_CHUNK_REGEX) || []; return matches.reduce( From ec2c791767e49257e2fb9edc84bdaeac01b1dbb8 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Thu, 10 Mar 2022 16:29:32 -0500 Subject: [PATCH 17/21] Added dep check --- build-system/test-configs/dep-check-config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/build-system/test-configs/dep-check-config.js b/build-system/test-configs/dep-check-config.js index 0440bf6631e7..17a50c8ee158 100644 --- a/build-system/test-configs/dep-check-config.js +++ b/build-system/test-configs/dep-check-config.js @@ -400,6 +400,7 @@ exports.rules = [ 'extensions/amp-story/1.0/amp-story-localization-service.js->src/service/localization/index.js', 'extensions/amp-story*/**/*.js->src/service/localization/strings.js', 'extensions/amp-story-auto-ads/0.1/story-ad-localization.js->src/service/localization/index.js', + 'extensions/amp-story/1.0/amp-story.js->src/service/extension-script.js', // Accessing calculateScriptBaseUrl() for vendor config URLs 'extensions/amp-analytics/0.1/config.js->' + From 3a5e409f7318b0064e67abbd67ed2afa8e9f2e82 Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Mon, 14 Mar 2022 10:51:55 -0400 Subject: [PATCH 18/21] Made tests more explicit --- extensions/amp-story/1.0/test/test-amp-story.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 677530000a00..6aa952892c96 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1738,7 +1738,7 @@ describes.realWin( await createStoryWithPages(1, ['cover']); - expect(fetchSpy).to.have.been.calledWith( + expect(fetchSpy).to.have.been.calledOnceWithExactly( 'https://cdn.ampproject.org/v0/amp-story.es-419.json' ); }); @@ -1754,7 +1754,7 @@ describes.realWin( await createStoryWithPages(1, ['cover']); - expect(fetchSpy).to.have.been.calledWith( + expect(fetchSpy).to.have.been.calledOnceWithExactly( '/dist/v0/amp-story.es-419.json' ); }); @@ -1765,11 +1765,10 @@ describes.realWin( const fetchMock = env.sandbox.mock(Services.xhrFor(env.win)); fetchMock.expects('fetchJson').returns( Promise.resolve({ - json: () => { - return Promise.resolve({ + json: () => + Promise.resolve({ '35': 'REMOTE-STRING', - }); - }, + }), }) ); From b3b7b17991bca5dee19eaf1082842866d02da38a Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Mon, 14 Mar 2022 10:58:37 -0400 Subject: [PATCH 19/21] Catching unknown language --- extensions/amp-story/1.0/amp-story.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index f7a87285435d..8360855e2b7a 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -41,6 +41,7 @@ import { setImportantStyles, toggle, } from '#core/dom/style'; +import {devError} from '#core/error'; import {isEsm} from '#core/mode'; import {findIndex, lastItem, toArray} from '#core/types/array'; import {debounce} from '#core/types/function'; @@ -2534,7 +2535,10 @@ export class AmpStory extends AMP.BaseElement { .then((res) => res.json()) .then((json) => localizationService.registerLocalizedStringBundle(languageCode, json) - ); + ) + .catch((err) => { + devError(TAG, err, 'Bundle not found for language ' + languageCode); + }); } } From 0900d282476be0e63223f2a8ab8946e9dbee0c4f Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 15 Mar 2022 11:05:29 -0400 Subject: [PATCH 20/21] Fixed comments from gmajoulet --- build-system/test-configs/forbidden-terms.js | 1 + extensions/amp-story/1.0/amp-story.js | 15 +++-- .../amp-story/1.0/test/test-amp-story.js | 60 +++++++++++-------- 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/build-system/test-configs/forbidden-terms.js b/build-system/test-configs/forbidden-terms.js index 4586d72c227b..bcd2e5fbf3f7 100644 --- a/build-system/test-configs/forbidden-terms.js +++ b/build-system/test-configs/forbidden-terms.js @@ -430,6 +430,7 @@ const forbiddenTermsGlobal = { 'build-system/externs/amp.extern.js', 'extensions/amp-subscriptions-google/0.1/amp-subscriptions-google.js', 'extensions/amp-video/0.1/video-cache.js', + 'extensions/amp-story/1.0/amp-story.js', 'src/utils/xhr-utils.js', ], }, diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js index 8360855e2b7a..9621f2d897d2 100644 --- a/extensions/amp-story/1.0/amp-story.js +++ b/extensions/amp-story/1.0/amp-story.js @@ -2448,17 +2448,21 @@ export class AmpStory extends AMP.BaseElement { /** * Adds the localization string bundles to the localization service. + * @private */ installLocalizationStrings_() { const localizationService = getLocalizationService(this.element); const storyLanguage = localizationService.getLanguageCodesForElement( this.element )[0]; - if (this.registerInlineLocalizationStrings_(storyLanguage)) { + if (this.maybeRegisterInlineLocalizationStrings_(storyLanguage)) { return; } - if (isExperimentOn(this.win, 'story-remote-locales')) { + if (isExperimentOn(this.win, 'story-remote-localization')) { this.fetchLocalizationStrings_(storyLanguage); + Services.performanceFor(this.win).addEnabledExperiment( + 'story-remote-localization' + ); } else { getLocalizationService(this.element) .registerLocalizedStringBundle('default', LocalizedStringsEn) @@ -2494,8 +2498,9 @@ export class AmpStory extends AMP.BaseElement { * If there are inline localization strings, register as current document language. * @param {string} languageCode * @return {boolean} + * @private */ - registerInlineLocalizationStrings_(languageCode) { + maybeRegisterInlineLocalizationStrings_(languageCode) { const inlineStringsEl = this.win.document.querySelector( 'script[amp-localization="amp-story"]' ); @@ -2521,6 +2526,7 @@ export class AmpStory extends AMP.BaseElement { /** * Fetches from the CDN or localhost the localization strings. * @param {string} languageCode + * @private */ fetchLocalizationStrings_(languageCode) { const localizationService = getLocalizationService(this.element); @@ -2530,8 +2536,9 @@ export class AmpStory extends AMP.BaseElement { getMode(this.win).localDev )}/v0/amp-story.${languageCode}.json`; + // The cache fallbacks to english if language not found, locally it errors. Services.xhrFor(this.win) - .fetchJson(localizationUrl) + .fetchJson(localizationUrl, {prerenderSafe: true}) .then((res) => res.json()) .then((json) => localizationService.registerLocalizedStringBundle(languageCode, json) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 6aa952892c96..7e810cb54157 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1710,36 +1710,44 @@ describes.realWin( ); }); - describe('remote locales', () => { + describe('remote localization strings', () => { beforeEach(() => { - toggleExperiment(env.win, 'story-remote-locales', true); + toggleExperiment(env.win, 'story-remote-localization', true); + }); + + afterEach(() => { + toggleExperiment(env.win, 'story-remote-localization', false); }); it('should fetch the localization strings for the default laguage from the cdn', async () => { - const fetchSpy = env.sandbox.spy( - Services.xhrFor(env.win), - 'fetchJson' - ); + const fetchStub = env.sandbox + .stub(Services.xhrFor(env.win), 'fetchJson') + .resolves({ + json: () => Promise.resolve({}), + }); await createStoryWithPages(1, ['cover']); - expect(fetchSpy).to.be.calledOnceWithExactly( - 'https://cdn.ampproject.org/v0/amp-story.en.json' + expect(fetchStub).to.be.calledOnceWithExactly( + 'https://cdn.ampproject.org/v0/amp-story.en.json', + env.sandbox.match.any ); }); it('should fetch the localization strings for the document laguage from the cdn', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es-419'); - const fetchSpy = env.sandbox.spy( - Services.xhrFor(env.win), - 'fetchJson' - ); + const fetchStub = env.sandbox + .stub(Services.xhrFor(env.win), 'fetchJson') + .resolves({ + json: () => Promise.resolve({}), + }); await createStoryWithPages(1, ['cover']); - expect(fetchSpy).to.have.been.calledOnceWithExactly( - 'https://cdn.ampproject.org/v0/amp-story.es-419.json' + expect(fetchStub).to.have.been.calledOnceWithExactly( + 'https://cdn.ampproject.org/v0/amp-story.es-419.json', + env.sandbox.match.any ); }); @@ -1747,37 +1755,37 @@ describes.realWin( env.win.document.body.parentElement.setAttribute('lang', 'es-419'); env.win.__AMP_MODE.localDev = true; - const fetchSpy = env.sandbox.spy( - Services.xhrFor(env.win), - 'fetchJson' - ); + const fetchStub = env.sandbox + .stub(Services.xhrFor(env.win), 'fetchJson') + .resolves({ + json: () => Promise.resolve({}), + }); await createStoryWithPages(1, ['cover']); - expect(fetchSpy).to.have.been.calledOnceWithExactly( - '/dist/v0/amp-story.es-419.json' + expect(fetchStub).to.have.been.calledOnceWithExactly( + '/dist/v0/amp-story.es-419.json', + env.sandbox.match.any ); }); it('should use the remote localization strings', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es-419'); - const fetchMock = env.sandbox.mock(Services.xhrFor(env.win)); - fetchMock.expects('fetchJson').returns( - Promise.resolve({ + const fetchStub = env.sandbox + .stub(Services.xhrFor(env.win), 'fetchJson') + .resolves({ json: () => Promise.resolve({ '35': 'REMOTE-STRING', }), - }) - ); + }); await createStoryWithPages(1, ['cover']); expect(localizationService.getLocalizedString('35')).to.be.equal( 'REMOTE-STRING' ); - fetchMock.verify(); }); }); }); From 59ebcb5dfe2ee0009ac2c1dc09ea79fbbbcb4dbf Mon Sep 17 00:00:00 2001 From: Matias Szylkowski Date: Tue, 15 Mar 2022 11:29:58 -0400 Subject: [PATCH 21/21] Remove unused fetchStub const, test with performance service --- extensions/amp-story/1.0/test/test-amp-story.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/extensions/amp-story/1.0/test/test-amp-story.js b/extensions/amp-story/1.0/test/test-amp-story.js index 7e810cb54157..bfdc03a8c2c8 100644 --- a/extensions/amp-story/1.0/test/test-amp-story.js +++ b/extensions/amp-story/1.0/test/test-amp-story.js @@ -1631,6 +1631,8 @@ describes.realWin( win.__AMP_MODE = { rtvVersion: '123', }; + const performanceImpl = new Performance(env.win); + env.sandbox.stub(Services, 'performanceFor').returns(performanceImpl); }); it('should install the default english localizations', async () => { @@ -1772,14 +1774,12 @@ describes.realWin( it('should use the remote localization strings', async () => { env.win.document.body.parentElement.setAttribute('lang', 'es-419'); - const fetchStub = env.sandbox - .stub(Services.xhrFor(env.win), 'fetchJson') - .resolves({ - json: () => - Promise.resolve({ - '35': 'REMOTE-STRING', - }), - }); + env.sandbox.stub(Services.xhrFor(env.win), 'fetchJson').resolves({ + json: () => + Promise.resolve({ + '35': 'REMOTE-STRING', + }), + }); await createStoryWithPages(1, ['cover']);