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

mszylkowski · 2022-03-09T17:23:29Z

Create experiment story-remote-localization that disables the localization strings from the bundle and relies on the remote strings (from the CDN or localhost). The story first tries to find inline strings, and if not found, it will either install the strings bundled into amp-story or it will fetch from the CDN or localhost (depending on localDev)

Strings can be from:

Inlined (on the AMP cache and transformers)
CDN (for origin stories, https://cdn.ampproject.org/v0/amp-story.en.json)
Localhost (for local dev, http://localhost:8000/dist/v0/amp-story.en.json)

Note: if the language is not one of the 80+ languages accepted, we will deal with that in the cache. Locally it will fail, but should not happen for any demo files.

dist/v0/amp-story-0.1.mjs: Δ +0.14KB
dist/v0/amp-story-1.0.mjs: Δ +0.14KB
dist/v0/amp-story-0.1.js: Δ +0.15KB
dist/v0/amp-story-1.0.js: Δ +0.15KB

cc @erwinmombay

Co-authored-by: Gabriel Majoulet <[email protected]>

…nto stringjson_inlined

…trings_controlled

amp-owners-bot · 2022-03-10T21:17:37Z

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story.js

extensions/amp-story/1.0/amp-story.js

extensions/amp-story/1.0/test/test-amp-story.js

mszylkowski

Fixed all comments, ready for another look @gmajoulet

extensions/amp-story/1.0/test/test-amp-story.js

mszylkowski · 2022-03-15T15:06:08Z

extensions/amp-story/1.0/test/test-amp-story.js

@@ -1709,6 +1709,77 @@ describes.realWin(
          'Swipe up'
        );
      });
+
+      describe('remote locales', () => {


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

extensions/amp-story/1.0/test/test-amp-story.js

extensions/amp-story/1.0/amp-story.js

gmajoulet · 2022-03-16T22:04:24Z

extensions/amp-story/1.0/amp-story.js

+      .then((json) =>
+        localizationService.registerLocalizedStringBundle(languageCode, json)
+      )
+      .catch((err) => {


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

mszylkowski · 2022-03-17T15:38:40Z

@samouri we need OWNERS approval to add a prerenderSafe request that fetches a JSON from the AMP cache. Do you need any more details on this? Request will be to https://cdn.ampproject.org/v0/amp-story.en.json but en can be replaced with other languages (eg: es) to fetch the localization strings.

ampprojectbot · 2022-03-17T18:59:12Z

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
If unsure, @-mention ampproject/wg-approvers

mszylkowski and others added 29 commits December 21, 2021 14:45

Added tasts

6aabe91

Undo

dc1fa87

Merge branch 'main' of github.com:ampproject/amphtml

d707e5b

Merge branch 'main' of github.com:ampproject/amphtml

0e3df75

Merge branch 'main' of github.com:ampproject/amphtml

a3f7bd0

Merge branch 'main' of github.com:ampproject/amphtml

8c5119c

Merge branch 'main' of github.com:ampproject/amphtml

77f8435

Merge branch 'main' of github.com:ampproject/amphtml

16e712e

Merge branch 'main' of github.com:ampproject/amphtml

8088eb7

Merge branch 'main' of github.com:ampproject/amphtml

3858b2a

Merge branch 'main' of github.com:ampproject/amphtml

e12d2f6

Merge branch 'main' of github.com:ampproject/amphtml

2a29105

Merge branch 'main' of github.com:ampproject/amphtml

a5b781f

Merge branch 'main' of github.com:ampproject/amphtml

b8006ee

Added logic and tests

a04f6d2

Updated tests with correct version

9c4f9b9

Updated comment

07e3ca5

Co-authored-by: Gabriel Majoulet <[email protected]>

Catch error in JSON inlined

879fa46

Merge branch 'stringjson_inlined' of github.com:mszylkowski/amphtml i…

5dfb927

…nto stringjson_inlined

Removed empty lines

bc9c7ed

Changed attribute to amp-localization

e53661a

Added json fetching for testing

55274f6

Remove .only

595c683

Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…

ed4ecaf

…trings_controlled

Install localization strings if inside experiment

5b923de

Merge branch 'main' of github.com:ampproject/amphtml into TEST_fetchs…

6f8c1ca

…trings_controlled

Added tests

1d419c3

Moved test to inside describes

72070cf

Remove unused function

1361273

mszylkowski requested a review from gmajoulet March 10, 2022 21:12

mszylkowski changed the title ~~[Draft] Test fetchstrings controlled~~ ✨ [Story localization] Fetch strings for localization files if in experiment Mar 10, 2022

mszylkowski marked this pull request as ready for review March 10, 2022 21:17

amp-owners-bot bot requested a review from samouri March 10, 2022 21:17

mszylkowski added 2 commits March 10, 2022 16:17

Remove unused import

5b49fff

Reverted index.js

b293243

mszylkowski removed the request for review from samouri March 10, 2022 21:19

mszylkowski added 3 commits March 10, 2022 16:29

Added dep check

ec2c791

Made tests more explicit

3a5e409

Catching unknown language

b3b7b17

mszylkowski mentioned this pull request Mar 14, 2022

✨ [Story localization] Localize system layer async #37870

Merged

gmajoulet reviewed Mar 14, 2022

View reviewed changes

Fixed comments from gmajoulet

0900d28

mszylkowski commented Mar 15, 2022

View reviewed changes

mszylkowski requested a review from gmajoulet March 15, 2022 15:07

Remove unused fetchStub const, test with performance service

59ebcb5

gmajoulet approved these changes Mar 16, 2022

View reviewed changes

mszylkowski requested review from caroqliu and samouri and removed request for caroqliu March 17, 2022 14:18

samouri approved these changes Mar 17, 2022

View reviewed changes

mszylkowski merged commit 32ea192 into ampproject:main Mar 17, 2022

mszylkowski deleted the TEST_fetchstrings_controlled branch March 17, 2022 18:44

ampprojectbot added the PR use: In Beta / Experimental label Mar 24, 2022

processprocess mentioned this pull request Mar 28, 2022

wg-stories Status Update 2022-03-28 ampproject/wg-stories#100

Open

ampprojectbot added the PR use: In Stable label Apr 4, 2022

ampprojectbot added the PR use: In LTS label Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mszylkowski commented Mar 9, 2022 •

edited

Loading

amp-owners-bot bot commented Mar 10, 2022

mszylkowski left a comment

mszylkowski Mar 15, 2022

gmajoulet Mar 16, 2022

mszylkowski commented Mar 17, 2022

ampprojectbot commented Mar 17, 2022

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

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

Conversation

mszylkowski commented Mar 9, 2022 • edited Loading

amp-owners-bot bot commented Mar 10, 2022

mszylkowski left a comment

Choose a reason for hiding this comment

mszylkowski Mar 15, 2022

Choose a reason for hiding this comment

gmajoulet Mar 16, 2022

Choose a reason for hiding this comment

mszylkowski commented Mar 17, 2022

ampprojectbot commented Mar 17, 2022

mszylkowski commented Mar 9, 2022 •

edited

Loading