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

feat: allow for tree-shaking unused langauges translations in custom apps #19799

Merged
merged 21 commits into from
Jan 9, 2025

Conversation

Platonn
Copy link
Contributor

@Platonn Platonn commented Dec 20, 2024

  • add a lot of missing index.ts files to re-export JSON translations in TS for new langauges
  • update translations.ts barrel files, to export each langauge in a separate public API members (to allow for treeshaking unused langauges). Note: it preservers the old feature prefix (e.g. asmTranslations -> asmTranslationsEn, asmTranslationsDe, ...)
  • deprecate the the old wrapper consts e.g. asmTranslations (and moved those deprecated tokens to public_api.ts files from translations.ts files)
  • remove from the old wrapper consts e.g. asmTranslations langauges other than en (in other words: revert the change made previously on the branch epic/tew-translation which added 3 other langauges into those wrapper objects)
  • add a new function extractTranslationChunksConfig() in @spartacus/core and use it in every library for creating chunks config in their translation.ts files. It extracts the chunks config from the translations assets automatically. So no more need to type chunks config manually.
  • fix our installation schematics, to use the new recommended syntax in the scaffolded features e.g.:
    - resources: asmTranslations,
    + resources: { en: asmTranslationsEn }

By the way:

  • noticed a broken chunks mapping for the cpqQuote feature. now should be fixed since using function extractTranslationChunksConfig()
  • noticed a typo in the existing old chunk name myAccountV2NotifiationPerference, HOWEVER I didn't fix it in this PR - it should be done in a separate PR
  • noticed yet another typo in existing file name my-account-v2-notification-perference.json (btw. it's a different typo than the above chunk name), HOWEVER I didnt fix it in this PR
  • noticed some i18n JSON files don't use camelCase, but kebab-case, HOWEVER I didn't fix it in this PR - it should be done in a separate PR

The reason I didn't want to fix those things by the way is:

  • it might affect the workflow of the TEW team, and I'd need to first double check with them
  • it's not related to this PR's main purpose (three-shaking)

TODO

  • fix unit tests and Jest snapshots in schematics
  • use it this way also in our storefrontapp
  • write doc input for the migration docs (to use specific langauges)

related to https://jira.tools.sap/browse/CXSPA-9131

QA steps

  1. checkout to this branch
  2. build libs
  3. publish to verdaccio
  4. create a new app npx @angular/cli@17 new test-app --standalone=false --ssr=false
  5. in the new app, install sparatcus from verdaccio ng add @spartacus/[email protected]. 0-1 --baseUrl="https://40.76.109. 9:9002" --ssr AND PLEASE SELECT ALL possible features and integrations in the interactive prompt
  6. In VsCode search everywhere (Cmd+Shift+F) for the phrase resources: and verify that everywhere in every feature module we use the new approach with explicit EN translations
  7. In some of those modules try to change the approach to old translations wrapper object and verify it's @deprecated (crossed out in VSCode)
  8. in spartacus-configuration.module.ts add also German translations for the main translations:
    i18n: {
         resources: { en: translationsEn, de: translationsDe },
  9. in spartacus-configuration.module.ts add this config (it's needed for later to run http-server)
provideConfig(<SiteContextConfig>{
   context: {
     baseSite: ['electronics-spa', 'apparel-uk-spa'],
     currency: ['USD', 'GBP'],
     language: ['en', 'de'],
   },
 }),
  1. ng build (or for interactive mode run npm run watch). Don't try ng serve --configuration=production - it doesn't have so well tree shaking as ng build
  2. npx http-server dist/test-app/browser (or for interactive mode run in another terminal npx nodemon --watch "./dist/test-app/browser" --exec "npx http-server './dist/$test-app/browser'" --ext "*"
  3. In Chrome open http://127.0.0.1:8080/ (8080! not 4200 port!)
  4. In devtools Sources tab do the search (Cmd+F)
  5. paste this search phrase ariaLabelSuggestions and verify that in the loaded main.js file there are only 2 langauges (EN and DE which you have enabled in steps above) with translations for this key
    image

@Platonn Platonn reopened this Dec 20, 2024
@pawelfras
Copy link
Contributor

pawelfras commented Dec 23, 2024

I encountered two issues when tried to build libs for QA purposes:

  • Error: Cannot find module '@spartacus/core'
    This PR inroduces to @spartacus/assets a new dependency to the @spartacus/core library in file projects/assets/src/translations/translations.ts:
    image
    This cause the mentioned error. Do we want to add this new dependency or rather handle
    extractTranslationChunksConfig in a different way, e.g. copy-paste similar logic to assets library?
  • after moving deprecated translations to public_api file, generate-translations-ts-2-json.ts is refering to missing object.
    generate-translations-ts-2-json.ts:8:10 - error TS2724: '"./src/translations/translations"' has no exported member named 'translations'. Did you mean 'translationsCs'?
    
    8 import { translations } from './src/translations/translations';
    
    could be handled by adding the following line to the script:
    + const translations: any = { <--- typed as `any` to satisfy TS. Otherwise we would have to copy the `TranslationResources` type (to not import it from `@spartacus/core`. 
    +   en: translationsEn,
    + };

import { ja } from './ja/index';
import { de } from './de/index';
import { zh } from './zh/index';
import { extractTranslationChunksConfig } from '@spartacus/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

major: this line causes the build error described here: #19799 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the this problem with the commit 058ce32

@pawelfras
Copy link
Contributor

pawelfras commented Dec 24, 2024

After tweaking mentioned issues locally I went through the QA steps and everything works as expected 🔥
I didn't provide any fixes in commits/PRs as they need additional discussion.

…ons object, to prevent dependency on @spartacus/core
… point to public_api in the dev tsconfig. But use /dist in tsconfig.lib.prod.json
…/core to point to public_api in the dev tsconfig. But use /dist in tsconfig.lib.prod.json"

This reverts commit d4f9aff.

WHY REVERTING: it was incorrect decision to do it
@Platonn Platonn marked this pull request as ready for review January 8, 2025 11:41
@Platonn Platonn requested review from a team as code owners January 8, 2025 11:41
Copy link
Contributor

Choose a reason for hiding this comment

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

query: is this file related to PR's subject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope.
After resolving conflicts in the license header (2024 vs 2025) in many files, those files got saved in my IDE and 2 things we applied:

  • prettier
  • removal of unused imports
    The second is the case in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

query: is this file related to PR's subject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@pawelfras pawelfras left a comment

Choose a reason for hiding this comment

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

QA done, LGTM

@Platonn Platonn merged commit 21f21c4 into epic/tew-translation Jan 9, 2025
12 of 14 checks passed
@Platonn Platonn deleted the epic/tew-translation--script-v2 branch January 9, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants