Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Playwright - ar.json Missing #1612

Closed
1 task done
ramadanomar opened this issue Aug 2, 2022 · 4 comments · Fixed by #1642
Closed
1 task done

Playwright - ar.json Missing #1612

ramadanomar opened this issue Aug 2, 2022 · 4 comments · Fixed by #1642
Assignees
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon
Projects

Comments

@ramadanomar
Copy link
Contributor

ramadanomar commented Aug 2, 2022

Description

No ar.json found when testing locales for Typescript on WSL

Reproduction

  1. clone the repo
  2. run pnpm dev
  3. After compilation i recieve this:
  4. See error.

Screenshots

image

Environment

  • Device: PC
  • OS: Windows 11 WSL v2 Ubuntu
  • Browser: Chrome

Additional context

To quote @sarayourfriend
The issue is that the ar.json file needs to be copied from the testing locales for TS to find them, but I think we should just @ts-ignore that line so that pnpm dev doesn't have to worry about tracking changes to the testing configuration.

Resolution

  • 🙋 I would be interested in resolving this bug. (I'd love to try)
@ramadanomar ramadanomar added 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work 🛠 goal: fix Bug fix labels Aug 2, 2022
@openverse-bot openverse-bot added this to Backlog in Openverse Aug 2, 2022
@ramadanomar ramadanomar changed the title <Replace this with actual title> Playwright - ar.json Missing Aug 2, 2022
@sarayourfriend sarayourfriend added 🟥 priority: critical Must be addressed ASAP 🤖 aspect: dx Concerns developers' experience with the codebase and removed 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Aug 2, 2022
@sarayourfriend
Copy link
Contributor

Just to confirm @ramadanomar, did you want to try opening a PR for this? I've assigned the issue to you in the meantime, please let me know if you're not interested and someone else can hop on this.

Thanks for reporting it and sorry for the trouble.

@sarayourfriend
Copy link
Contributor

Also: I believe this is not an issue with WSL but any fresh clone of the repository.

@openverse-bot openverse-bot moved this from Backlog to In progress in Openverse Aug 3, 2022
@obulat obulat added 🟨 priority: medium Not blocking but should be addressed soon and removed 🟥 priority: critical Must be addressed ASAP labels Aug 3, 2022
@obulat
Copy link
Contributor

obulat commented Aug 3, 2022

I've lowered the priority for this issue to medium because the problem has a workaround:

  1. Run pnpm i18n. This should download all the translations and locale files.
  2. Run pnpm dev.

The issue occurs when you run pnpm dev in a cleanly-installed repository. You have no locale files downloaded, and pnpm dev only creates the file with the list of locales (src/locales/scripts/valid-locales.json) without downloading the translations. So, when the navigation.ts tries to import ar.json translation, it raises an error.

The solution of un-ignoring the ar.json file from git (in #1619) would mean that every time a new Arabic string is translated, we'll have to commit the new changes to the repository. The git history will be full of unrelated changes.

We could also just run pnpm i18n (without the no-get) in pnpm dev, but that would mean that we send network requests to https://translate.wordpress.org every time someone runs pnpm dev locally. This is unnecessary and might cause 429 errors.

The best solution I could come up with would be to add flags to the i18n locale strings that enable conditional loading of the translations.
The i18n script would always run get-translations, both for build and dev commands.
However, for dev command (maybe with a no-get flag?), it would check if ar.json and valid-locale files exist. If so, it would return without downloading anything. Otherwise, it would download the translations and the locales.

@sarayourfriend
Copy link
Contributor

The TypeScript error occurs in the Playwright tests. Can we @ts-ignore that line with a note saying that the file sometimes doesn't exist?

I agree that copying the testing ar.json or adding the real one to the git history is not an appropriate solution, for the reasons you listed.

@obulat obulat mentioned this issue Aug 11, 2022
7 tasks
Openverse automation moved this from In progress to Done! Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon
Projects
No open projects
Openverse
  
Done!
3 participants