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

✨ [Story localization] Fetch strings for localization files if in experiment #37843

Merged
merged 36 commits into from
Mar 17, 2022
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
6aabe91
Added tasts
mszylkowski Dec 21, 2021
dc1fa87
Undo
mszylkowski Dec 21, 2021
d707e5b
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Dec 28, 2021
0e3df75
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Dec 28, 2021
a3f7bd0
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 4, 2022
8c5119c
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 6, 2022
77f8435
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 10, 2022
16e712e
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 19, 2022
8088eb7
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 25, 2022
3858b2a
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 28, 2022
e12d2f6
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Jan 31, 2022
2a29105
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Feb 1, 2022
a5b781f
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Feb 10, 2022
b8006ee
Merge branch 'main' of github.com:ampproject/amphtml
mszylkowski Mar 8, 2022
a04f6d2
Added logic and tests
mszylkowski Mar 8, 2022
9c4f9b9
Updated tests with correct version
mszylkowski Mar 8, 2022
07e3ca5
Updated comment
mszylkowski Mar 9, 2022
879fa46
Catch error in JSON inlined
mszylkowski Mar 9, 2022
5dfb927
Merge branch 'stringjson_inlined' of github.com:mszylkowski/amphtml i…
mszylkowski Mar 9, 2022
bc9c7ed
Removed empty lines
mszylkowski Mar 9, 2022
e53661a
Changed attribute to amp-localization
mszylkowski Mar 9, 2022
55274f6
Added json fetching for testing
mszylkowski Mar 9, 2022
595c683
Remove .only
mszylkowski Mar 9, 2022
ed4ecaf
Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…
mszylkowski Mar 10, 2022
5b923de
Install localization strings if inside experiment
mszylkowski Mar 10, 2022
6f8c1ca
Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…
mszylkowski Mar 10, 2022
1d419c3
Added tests
mszylkowski Mar 10, 2022
72070cf
Moved test to inside describes
mszylkowski Mar 10, 2022
1361273
Remove unused function
mszylkowski Mar 10, 2022
5b49fff
Remove unused import
mszylkowski Mar 10, 2022
b293243
Reverted index.js
mszylkowski Mar 10, 2022
ec2c791
Added dep check
mszylkowski Mar 10, 2022
3a5e409
Made tests more explicit
mszylkowski Mar 14, 2022
b3b7b17
Catching unknown language
mszylkowski Mar 14, 2022
0900d28
Fixed comments from gmajoulet
mszylkowski Mar 15, 2022
59ebcb5
Remove unused fetchStub const, test with performance service
mszylkowski Mar 15, 2022
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
1 change: 1 addition & 0 deletions build-system/test-configs/dep-check-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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->' +
Expand Down
110 changes: 78 additions & 32 deletions extensions/amp-story/1.0/amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -53,6 +54,7 @@ import {getHistoryState as getWindowHistoryState} from '#core/window/history';
import {isExperimentOn} from '#experiments';

import {Services} from '#service';
import {calculateScriptBaseUrl} from '#service/extension-script';
import {createPseudoLocale} from '#service/localization/strings';

import {getDetail} from '#utils/event-helper';
Expand Down Expand Up @@ -318,34 +320,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]`)
);
this.registerInlineLocalizationStrings_();
this.installLocalizationStrings_();

if (this.isStandalone_()) {
this.initializeStandaloneStory_();
Expand Down Expand Up @@ -2471,28 +2446,99 @@ export class AmpStory extends AMP.BaseElement {
}
}

/**
* Adds the localization string bundles to the localization service.
*/
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
installLocalizationStrings_() {
const localizationService = getLocalizationService(this.element);
const storyLanguage = localizationService.getLanguageCodesForElement(
this.element
)[0];
if (this.registerInlineLocalizationStrings_(storyLanguage)) {
return;
}
if (isExperimentOn(this.win, 'story-remote-locales')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Plz report the experiment as enabled to the CSI metrics so we can filter our dashboards and measure performance

Copy link
Contributor

Choose a reason for hiding this comment

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

Plz report the experiment as enabled to the CSI metrics so we can filter our dashboards

this.fetchLocalizationStrings_(storyLanguage);
} 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.
* @param {string} languageCode
* @return {boolean}
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
*/
registerInlineLocalizationStrings_() {
registerInlineLocalizationStrings_(languageCode) {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
const inlineStringsEl = this.win.document.querySelector(
'script[amp-localization="amp-story"]'
);
if (
inlineStringsEl?.getAttribute('i-amphtml-version') !==
getMode(this.win).rtvVersion
) {
return;
return false;
}
const stringsOrNull = tryParseJson(inlineStringsEl.textContent);

if (!stringsOrNull) {
return;
return false;
}
const localizationService = getLocalizationService(this.element);
localizationService.registerLocalizedStringBundle(
localizationService.getLanguageCodesForElement(this.element)[0],
languageCode,
stringsOrNull
);
return true;
}

/**
* Fetches from the CDN or localhost the localization strings.
* @param {string} languageCode
*/
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
fetchLocalizationStrings_(languageCode) {
const localizationService = getLocalizationService(this.element);

const localizationUrl = `${calculateScriptBaseUrl(
this.win.location,
getMode(this.win).localDev
)}/v0/amp-story.${languageCode}.json`;

Services.xhrFor(this.win)
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
.fetchJson(localizationUrl)
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
.then((res) => res.json())
.then((json) =>
localizationService.registerLocalizedStringBundle(languageCode, json)
)
.catch((err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this happen, if the AMP Cache falls back to english for an unknown languageCode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally you could create a demo with a language that doesn't exist and this will show in the console. Also the cache doesn't do this now, but will be implemented soon. Also if the language doesn't have a valid format (let's say someone uses ../../.. or en/us or some other complex string that the cache might not be able to fallback due to the path being broken).

All these can avoid breaking the code by catching the error. We could use regex for some of these, but in case anything goes wrong, catching the error makes the most sense IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't launch this till the AMP Cache does this fallback then, otherwise all docs with wrong lang parameters would start failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can trigger the experiment flag when the cache works better. This catch is still useful if the language breaks the url, the network drops, etc. Having a regex (\w+(-\w+)*) would fix the former but not the latter.

mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
devError(TAG, err, 'Bundle not found for language ' + languageCode);
});
}
}

Expand Down
71 changes: 71 additions & 0 deletions extensions/amp-story/1.0/test/test-amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,77 @@ describes.realWin(
'Swipe up'
);
});

describe('remote locales', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in the main code file, please be consistent with your vocabulary. locales/localizationStrings etc, please pick one and stick to it for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code overall doesn't seem to be extremely consistent but will try with this PR to keep all the changes using localization.

mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(() => {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
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(
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
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');

const fetchSpy = env.sandbox.spy(
Services.xhrFor(env.win),
'fetchJson'
);

await createStoryWithPages(1, ['cover']);

expect(fetchSpy).to.have.been.calledOnceWithExactly(
'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.calledOnceWithExactly(
'/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({
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
json: () =>
Promise.resolve({
'35': 'REMOTE-STRING',
}),
})
);

await createStoryWithPages(1, ['cover']);

expect(localizationService.getLocalizedString('35')).to.be.equal(
'REMOTE-STRING'
);
fetchMock.verify();
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
}
);