cal.com icon indicating copy to clipboard operation
cal.com copied to clipboard

chore: i18n cleanup - script to remove unused translation keys

Open thepradipvc opened this issue 1 year ago โ€ข 6 comments

What does this PR do?

  • Fixes #15623
  • /claim #15623

This PR adds script to remove the unused translation keys from i18n translation files.

Mandatory Tasks (DO NOT REMOVE)

  • [X] I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • [ ] I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  1. Run the script using yarn web remove-unused-translations.
  2. All the unused translations will be removed except those in KEYS_TO_IGNORE.

I've tested my regex by creating a single file including all the patterns in which the translations are used in code and it works perfectly. However, there is one big consideration which I will discuss in comments due to which the keys generated dynamically in code can not be saved from removal without adding manual comments in the code as I will describe in details in this PR's comments.

thepradipvc avatar Aug 09 '24 14:08 thepradipvc

@thepradipvc is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Aug 09 '24 14:08 vercel[bot]

๐Ÿ’ต To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

algora-pbc[bot] avatar Aug 09 '24 14:08 algora-pbc[bot]

This is the regex that I've created but I'm not accounting for dynamically generated keys in this due to reasons discussed below.

/**
 * # Regex for translation keys
 * This regex matches the following function calls and attribute:
 * 1. t("<some_key>"),
 * 2. t(`<some_key>`),
 * 3. t("<some_key>", {key: "<some_string>"}),
 * 4. t(`<some_key>`, {key: "<some_string>"}),
 * 5. i18nKey="home_hero_subtitle",
 * 6. i18nKey={`home_hero_subtitle`},
 *
 * It also ensure that we don't match any other similar function calls (e.g. `format("dddd")`).
 **/
const translationKeyRegex =
  /(?<!\w)(?:(?:t|language)\((?:"|`)([^$"]*)(?:"|`)(?:,\s*\{[^}]*\})?\)|i18nKey=(?:{`|")([^$"]*)(?:`}|")[^\w])/g;

booking_${needsConfirmation ? "submitted" : "confirmed"}${recurringBookings ? "_recurring" : ""} I'm not using regex to find used keys in dynamically generated keys due to the following reasons.

  1. It is possible to extract all the possible combinations of generated keys in above example but it will get very complex because of other patterns in codebase where at some places the variables are even coming as props like in following example. delete${isManagedEvent}_event_type.
  2. We can create a regex to keep keys in translations that match above pattern like <something>_event_type, but this is also not a viable option as there are several keys for which only one variable is used meaning that it will match all the translation keys and we can not remove any key. Example - ${key.toString().toLowerCase()}, ${trigger.toLowerCase()}, ${template.toLowerCase()} and few more.

My suggestion: We keep the possible keys that can be generated in comments whenever the key string is generated dynamically like following.

const titleSuffix = props.recurringBookings ? "_recurring" : "";
-----------------
// Assume as collapsed code
-----------------
if (needsConfirmation) {
  if (props.profile.name !== null) {
     /* Possible keys that will be generated
         t(`user_needs_to_confirm_or_reject_booking`)
         t(`user_needs_to_confirm_or_reject_booking_recurring`) */
    return t(`user_needs_to_confirm_or_reject_booking${titleSuffix}`, {
      user: props.profile.name,
    });
  }
    /* Possible keys that will be generated
        t(`needs_to_be_confirmed_or_rejected`)
        t(`needs_to_be_confirmed_or_rejected_recurring`)  */
  return t(`needs_to_be_confirmed_or_rejected${titleSuffix}`);
}

There are only about 60 places where the translation key is generated dynamically which can easily be covered in a day to write comments then it has to be mainted in future whenever someone writes code that dynamically generates translation key.

Open this to view all the dynamic key generation code in repo
[
  "booking_${needsConfirmation ? 'submitted' : 'confirmed'}${recurringBookings ? '_recurring' : ''}",
  "${lockedText}_${isManagedEventType ? 'for_members' : 'by_team_admins'}",
  "delete${isManagedEvent}_event_type",
  "delete${isManagedEvent}_event_type_description",
  "${key.toString().toLowerCase()}",
  "user_needs_to_confirm_or_reject_booking${titleSuffix}",
  "needs_to_be_confirmed_or_rejected${titleSuffix}",
  "${titlePrefix}emailed_you_and_attendees${titleSuffix}",
  "emailed_you_and_attendees${titleSuffix}",
  "delete${isManagedEventPrefix()}_event_type",
  "delete${isManagedEventPrefix()}_event_type_description",
  "404_the_${currentPageType.toLowerCase()}",
  "404_claim_entity_${currentPageType.toLowerCase()}",
  "no_category_apps_description_${variant || 'other'}",
  "connect_${variant || 'other'}_apps",
  "installed_app_${variant || 'other'}_description",
  "Cal will provide a ${videoLocation.label} URL.",
  "You have created ${props.orgName} organization.",
  "verify_email_subject${props.isVerifyingEmail ? '_verifying_email' : ''}",
  "org_team_names_example_${index + 1}",
  "${workflow.steps[0].action.toLowerCase()}_action",
  "${option.toLowerCase()}_timeUnit",
  "${workflow.timeUnit.toLowerCase()}",
  "${workflow.trigger.toLowerCase()}_trigger",
  "${trigger.toLowerCase()}_trigger",
  "${step.action.toLowerCase()}_action",
  "${step.template.toLowerCase()}",
  "${action.toLowerCase()}_action",
  "${triggerEvent.toLowerCase()}_trigger",
  "${template.toLowerCase()}",
  "${formattedVar}variable",
  "bookerlayout_${layout}",
  "${trigger.toLowerCase()}",
  "${variable}_variable",
  "${variable}_info",
  "${selectedOption.name}_variable",
  "${option.name}_variable",
  "${option.name}_info",
  "${fieldName}_hint_${key}",
  "${props.name}_placeholder",
  "${name}_placeholder",
  "${props.orgSlug ? 'org' : 'team'}_is_unpublished_description"
]

thepradipvc avatar Aug 09 '24 14:08 thepradipvc

Graphite Automations

"Add consumer team as reviewer" took an action on this PR โ€ข (08/09/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR โ€ข (08/09/24)

1 label was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR โ€ข (08/29/24)

1 reviewer was added to this PR based on Keith Williams's automation.

graphite-app[bot] avatar Aug 09 '24 14:08 graphite-app[bot]

For the keys used in migrations files.

So far there are only few used keys found in migration files as discussed in other PRs.

Toggle to see the usage in migration files
-- Seed data for stock reasons
INSERT INTO "OutOfOfficeReason" ("emoji", "reason", "enabled") VALUES ('๐Ÿ•’', 'ooo_reasons_unspecified', true);
INSERT INTO "OutOfOfficeReason" ("emoji", "reason", "enabled") VALUES ('๐Ÿ๏ธ', 'ooo_reasons_vacation', true);
INSERT INTO "OutOfOfficeReason" ("emoji", "reason", "enabled") VALUES ('๐Ÿ›ซ', 'ooo_reasons_travel', true);
INSERT INTO "OutOfOfficeReason" ("emoji", "reason", "enabled") VALUES ('๐Ÿค’', 'ooo_reasons_sick_leave', true);
INSERT INTO "OutOfOfficeReason" ("emoji", "reason", "enabled") VALUES ('๐Ÿ“…', 'ooo_reasons_public_holiday', true);

So should we just add these in the KEYS_TO_IGNORE array in the script. OR If it's unsure whether other keys are used in migration files, we can check for each key in our translation files if it's present in any .sql (migration) files using fileContent.includes(key) but this would be a bit slow.

thepradipvc avatar Aug 09 '24 14:08 thepradipvc

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Aug 24 '24 00:08 github-actions[bot]

This PR is being marked as stale due to inactivity.

github-actions[bot] avatar Sep 14 '24 00:09 github-actions[bot]

Thanks for the contribution @thepradipvc but we have decided not to pursue this.

Amit91848 avatar Sep 18 '24 05:09 Amit91848

Thanks for the contribution @thepradipvc but we have decided not to pursue this.

No problem Great decision btw it was too risky and would have needed a lot of maintenance creating a lot of inconvenience for a little convenience at other place.

thepradipvc avatar Sep 18 '24 05:09 thepradipvc