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

extensions/amp-story/1.0/_locales/af.json
extensions/amp-story/1.0/_locales/am.json
extensions/amp-story/1.0/_locales/ar.json
extensions/amp-story/1.0/_locales/bg.json
extensions/amp-story/1.0/_locales/bn.json
extensions/amp-story/1.0/_locales/bs.json
extensions/amp-story/1.0/_locales/ca.json
extensions/amp-story/1.0/_locales/cs.json
extensions/amp-story/1.0/_locales/da.json
extensions/amp-story/1.0/_locales/de.json
extensions/amp-story/1.0/_locales/default.json
extensions/amp-story/1.0/_locales/el.json
+59 more

@danielrozenberg do you know why the bundle-size check is still pending even after several days? Given the number of lines changed here, we'd want to check that before approving

@danielrozenberg do you know why the bundle-size check is still pending even after several days? Given the number of lines changed here, we'd want to check that before approving

Looks like the bundle-size job was cancelled because of a failure in unit tests - I recommend rebasing on main and force-pushing. We just removed the fail-fast behaviour, so even if there are failures in other jobs, the bundle-size job will continue

Thanks @danielrozenberg!

@ilyaspiridonov do you mind rebasing and force pushing as Daniel mentions above? Thanks!

@newmuis any idea why circleCI is failing?

@calebcordry do you know (since it's an amp-story-auto-ads E2E test that is failing)?

I restarted the test as it looks like a flake. That should do it.

The bundle-size saw a 5.37KB increase which is rather large. Can we look into easy options to make this better?
The first idea that comes to mind would be removing the "description" from non en.json files as it's not used. This should be very impactful. I'll let @newmuis suggest a way forward here.

Agreed that removing descriptions from every file except for en.json would be helpful.

@ilyaspiridonov would you mind doing that so we can re-evaluate the bundle size?

Ping @ilyaspiridonov, do you mind removing the descriptions from the non-English translations for now?

@newmuis Apologies for the long wait (PTO and traveling) - should be good now, but let's see what the bundle-size outputs.

Could you please add a new line at the end of each file? It looks like our systems are complaining about this.
Thanks!

We deprecated a feature recently and will need to remove these 4 i18n strings from these files:
AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',

Is it possible to make this change in this PR?

We deprecated a feature recently and will need to remove these 4 i18n strings from these files:
AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',

Is it possible to make this change in this PR?

I don't think they are in these files, am I missing something? @processprocess

AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',
Is it possible to make this change in this PR?

I don't think they are in these files, am I missing something? @processprocess

My bad, to clarify:
We need the strings keyed to 37, 18, 38 and 20 removed in each file.

@ilyaspiridonov are you able to provide translations for Portuguese (as spoken in Portugal, rather than Brazil) and Slovak?

The PR is currently failing because these files are expected to exist. If we don't want them to exist, we'll need to deprecate them, which will be a whole separate process.

Thanks for following through with this! Getting the first PR through will be the hardest; once we've established the right structure, hopefully we can continue to replicate it for future batches.

@processprocess let's remove these in a followup PR to make this one easier to get through

@processprocess let's remove these in a followup PR to make this one easier to get through

👍 Filed #35934 for tracking.

@newmuis Sorry, something must have happened to pt-pt, sl and et on my end, we did have these languages, just added them in this PR.
I also removed strings 37 18 38 20 in this PR, PTAL @processprocess

Thanks for the updates @ilyaspiridonov!

Looks like it's almost there, but the new files reintroduced formatting issues; do you mind updating those as well?

@newmuis Thank for the heads-up, the checks seem to be green now!

Thanks for working through this! Looks like it's passing the code checks now, and we can calculate the bundle size changes:

dist/v0/amp-story-0.1.mjs: Δ +3.12KB
dist/v0/amp-story-1.0.mjs: Δ +3.12KB
dist/v0/amp-story-0.1.js: Δ +3.03KB
dist/v0/amp-story-1.0.js: Δ +3.03KB

/to @gmajoulet for analysis on whether that increase is reasonable.

A few things to feel better about adding these extra 3KB:

• our users actually need these
• we just removed 2kB here https://github.com/ampproject/amphtml/runs/3558679416
• lazy loading the translation strings isn't straightforward as some are required for the initial render
• there are easier opportunities for impact: some huge chunks of code could easily be lazy loaded (sharing, page attachments, etc)

cc @ampproject/wg-performance FYI, I'll wait a bit in case you want to chime in before merging

@gmajoulet: FMI Is the current status quo to load all languages instead of just one? have we done any research to determine the size savings from only loading 1?

Loading just one today would mean either:

1. Lazy loading from the JS, hurting LCP and UX
2. Write an AMP transformer to add a tag to load the correct language, which would have my vote but it's work we haven't prioritized. I can track it somewhere

Have we done research to determine the size savings from only loading 1? That would help determine priority.

They're 3+KBs out of 70 so it'd be a 5-10% reduction in bundle size.

+1 in my opinion not a reason to block this current work, but also +1 to having some sort of tracking for a "real" fix of how to only serve the language the user needs. The 3 KB is the diff from what we already have, but I'd expect we'd shave more than 3 KB if we could find a way to only serve one language without the PX hit.

(A complex strawman: separately define render-blocking strings vs. non-render-blocking strings. Only bundle the render-blocking ones, late-load the rest, which AFAIK would/could be the majority). A transformer could do a lot (e.g. we know the quiz/poll strings are render-blocking if there is a quiz/poll on the first page of the story), but there may also be diminishing returns here.

Separate from any particular proposal, @gmajoulet do you mind filing a tracking issue for finding high-level alignment on a solution here?

I'm tracking various performance improvement project that I'm trying to sort by category (LCP/bundle-size/runtime performance etc). I'll finalize all of that and track it publicly as part of a larger bundle-size project.

@samouri translation/localization would be very SSR-able with potential to avoid any render-blocking caused by waiting for the strings to load.

Simple option is having a split out critical-path-strings.js etc that's much smaller, inlined, w/e