SalesforceCommerceCloud / pwa-kit

React-based JavaScript frontend framework to create a progressive web app (PWA) storefront for Salesforce B2C Commerce.
https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/pwa-kit-overview.html
BSD 3-Clause "New" or "Revised" License
280 stars 131 forks source link

[BUG] Doubts and possible issue with translations setup #1101

Open breadadams opened 1 year ago

breadadams commented 1 year ago

Summary

We've recently ran into a problem with the translations, and I would like to verify what the ideal/proposed workflow is, in regards to adding new localised strings. Should we be running the translation scripts (i.e. extract-default-translations) every time a localisation is added/modified?

The problem we've just faced was that we had updated a few of the localised strings from the template, and later compiled the default en-US locale. Fast forward to now, and we've added some new translated strings. We're getting tons of warnings in tests because these messages are missing for the default locale (which is different in tests, en-GB, see below)

To solve this I extracted the default translations (en-US), copied them into the en-GB file, and compiled them both, and all of a sudden numerous tests are failing. Turns out that these tests depended on some of the template strings that we'd updated weeks ago, however it was missed because the tests had continued to pass since the correct string was still there in the en-GB translation file until I'd just replaced them.

So the issue I see here is that the default locale for the project (and the translation script) is en-US, but then in the tests the default locale is en-GB. Shouldn't they match?

https://github.com/SalesforceCommerceCloud/pwa-kit/blob/6b9d4369b1460023de233e8b60b31feda38ebf55/packages/template-retail-react-app/app/constants.js#L11

https://github.com/SalesforceCommerceCloud/pwa-kit/blob/6b9d4369b1460023de233e8b60b31feda38ebf55/packages/template-retail-react-app/app/utils/test-utils.js#L28

If I'm not mistaken, the workflow given this setup would be the following:

  1. Modify localisation at code level.
  2. Execute extract-default-translations to update the .../translations/en-US.json file.
  3. Copy the contents of the above file into .../translations/en-GB.json.
  4. Run the compile-translations script to build .../translations/compiled/en-US.json and .../translations/compiled/en-GB.json.

However, and possibly most importantly, those above steps won't be applicable to a project that actually has different strings across US and GB. Take for example if I had a Hey {user} in US locale, and in a test assert that it's on the page. If the translators update GB to say Hello {user}, the tests would fail.

Any insight you can provide to clarify this would be much appreciated! 🙂

Steps To Reproduce

N/A

Expected result

N/A

Actual result

N/A

System Information (as applicable)

Browser: Node version: pwa-kit version: Desktop OS: Mobile Device Info:

vmarta commented 1 year ago

Should we be running the translation scripts (i.e. extract-default-translations) every time a localisation is added/modified?

Not only that extract script, but also the compile one. Because retail-react-app would try to grab the compiled translations: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/cf16f4611d3670bfdb151e5d0b94cb2824f1f7ad/packages/template-retail-react-app/app/utils/locale.js#L26

So for convenience, please run this script instead (to run both the extract and compile): npm run build-translations.

(But of course, feel free to call each script individually, as long as the translations get compiled at the end)

vmarta commented 1 year ago

About the inconsistency you found in the test setup, the DEFAULT_LOCALEs should match. At some point, it was changed from en-GB to en-US. But the tests were probably not updated accordingly. I'll look into this further.

git2gus[bot] commented 1 year ago

This issue has been linked to a new work item: W-12946757

breadadams commented 1 year ago

Hey @vmarta , thanks for your response.

Not only that extract script, but also the compile one

Yeah my bad, I meant the "full flow" of both scripts, so perfect 👍 .

[...] the DEFAULT_LOCALEs should match. At some point, it was changed from en-GB to en-US. But the tests were probably not updated accordingly. I'll look into this further.

Ok great, I look forward to hearing news on the matter.