fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

ahtesham-quraish · 2025-08-12T08:04:18Z

Description

Replace custom language names list with standard Intl DisplayNames API #2332

Supporting information

I have used iso-639-1, the reason of using this package is that the Intl.DisplayNames returns country code for many countries like ab: Abkhazian so I have to use fallback.

Testing instructions

Please provide detailed step-by-step instructions for manually testing this change.

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

Best Practices Checklist

We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:

Any new files are using TypeScript (.ts, .tsx).
Deprecated propTypes, defaultProps, and injectIntl patterns are not used in any new or modified code.
Tests should use the helpers in src/testUtils.tsx (specifically initializeMocks)
Do not add new fields to the Redux state/store. Use React Context to share state among multiple components.
Use React Query to load data from REST APIs. See any apiHooks.ts in this repo for examples.
All new i18n messages in messages.ts files have a description for translators to use.
Imports avoid using ../. To import from parent folders, use @src, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

codecov · 2025-08-12T08:18:53Z

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.60%. Comparing base (0e1550a) to head (e235984).
⚠️ Report is 29 commits behind head on master.

Files with missing lines	Patch %	Lines
...l/components/TranscriptWidget/LanguageSelector.jsx	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2371      +/-   ##
==========================================
+ Coverage   94.44%   94.60%   +0.15%     
==========================================
  Files        1169     1174       +5     
  Lines       25102    25428     +326     
  Branches     5473     5571      +98     
==========================================
+ Hits        23708    24056     +348     
+ Misses       1328     1305      -23     
- Partials       66       67       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bradenmacdonald

Thanks for this! Could you please finish updating the PR description? I was hoping you could explain some of your decisions a bit, like why you added the new iso-639-1 dependency.

bradenmacdonald · 2025-08-19T00:56:54Z

package.json

@@ -18,7 +18,7 @@
    "start": "fedx-scripts webpack-dev-server --progress",
    "start:with-theme": "paragon install-theme && npm start && npm install",
    "dev": "PUBLIC_PATH=/authoring/ MFE_CONFIG_API_URL='http://localhost:8000/api/mfe_config/v1' fedx-scripts webpack-dev-server --progress --host apps.local.openedx.io",
-    "test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",
+    "test": "TZ=UTC fedx-scripts jest --passWithNoTests",


Suggested change

"test": "TZ=UTC fedx-scripts jest --passWithNoTests",

"test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",

I think you committed this accidentally? I find the --coverage check here to be annoying as well, but we can't just change it without a dedicated PR and a bit more discussion.

yes it was done accidentally

bradenmacdonald · 2025-08-19T00:58:53Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

 import { FileInput, fileInput } from '../../../../../../sharedComponents/FileInput';
+import { getLanguageName } from '../../../../../../data/constants/video';


Suggested change

import { getLanguageName } from '../../../../../../data/constants/video';

import { getLanguageName } from '@src/editors/data/constants/video';

This is simpler - see the best practices checklist in the PR description.

Thanks I have updated it

bradenmacdonald · 2025-08-19T00:59:52Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

@@ -45,27 +45,27 @@ export const hooks = {
 };

 const LanguageSelector = ({
-  index, // For a unique id for the form control


Please leave this comment in place; I think it's helpful.

bradenmacdonald · 2025-08-19T01:00:15Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

  language,
-  // Redux
-  openLanguages, // Only allow those languages not already associated with a transcript to be selected
+  openLanguages,


Please add back a comment to explain this parameter.

bradenmacdonald · 2025-08-19T01:02:58Z

...eoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.test.jsx

@@ -46,13 +49,4 @@ describe('LanguageSelector', () => {
    render(<LanguageSelector {...props} language="" />);
    expect(screen.getByText('Select Language')).toBeInTheDocument();
  });
-
-  test('transcripts no Open Languages, all dropdown items should be disabled', () => {


Why is this test no longer needed?

This is reason we dont have disable option in our current implementation that is reason I have removed it

bradenmacdonald · 2025-08-19T01:04:49Z

src/editors/data/constants/video.js

-  za: 'Zhuang',
-  zu: 'Zulu',
-});
+export const getLanguageName = (langCode, locales = ['en']) => {


If you can't convert this file to TypeScript yet, please move this function to a separate language-utils.ts file that is using TypeScript so that you can document the types properly. Any significant new code like this should have types defined. (And if you can't move it to a .ts file or convert this file to .ts you can always add JSDoc type comments.)

Let me add JSDoc type comment because for now I am unable to move it to separate ts file but I will keep in mind to move it to different file later on.

bradenmacdonald · 2025-08-19T01:08:45Z

src/editors/data/constants/video.js

+export const openLanguagesDataSet = ISO6391.getAllCodes().reduce((acc, code) => {
+  acc[code] = ISO6391.getName(code);
+  return acc;
+}, {});

 export const in8lTranscriptLanguages = (intl) => {


We don't need this in8lTranscriptLanguages code anymore. You can already use getLanguageName (the Intl.DisplayNames API) to get the name of a language in any other language.

this is being used in transcriptLanguages function that is why I did not remove it.

Can you change transcriptLanguages to use getLanguageName instead, and then delete this?

bradenmacdonald · 2025-08-19T01:09:28Z

src/editors/data/constants/video.js


 export const in8lTranscriptLanguages = (intl) => {
  const messageLookup = {};
-  // for tests and non-internationlized setups, return en
+
+  // For tests and non-internationalized setups, return raw dataset


Node.js supports Intl.DisplayNames so it should work in tests too.

Sorry I did not get this point can you please expalin?

It doesn't matter if we can delete this whole function. But I was saying we don't need to have special behavior "for tests" because Intl.DisplayNames works in tests too so we can use the same code in tests as in the rest of the app.

bradenmacdonald · 2025-08-19T01:09:44Z

src/editors/data/constants/video.test.js

+  });
+
+  it('returns language name from Intl.DisplayNames for given locale', () => {
+    displayNamesSpy = jest.spyOn(global.Intl, 'DisplayNames').mockImplementation(() => ({


I don't think you need to mock it?

it is needed otherwise the code does not return the value I have explored by not mocking them but it was not working properly so I had to mock it

Can you give an example?

bradenmacdonald · 2025-08-19T01:11:18Z

src/editors/data/constants/video.js

+      // Fallback to ISO6391 if Intl.DisplayNames fails
+    }
+  }
+  const isoName = ISO6391.getName(code);


If we include ISO6391 as a dependency, it undoes the bundle size savings of removing the language names from our code. Did you find a case where the Intl.DisplayNames API was not working, and this fallback is necesesary?

I think we should just fall back to (A) Intl.DisplayNames response but for English instead of the user's locale, and (B) if that doesn't work, just return the language code as the language name.

I have used iso-639-1, the reason of using this package is that the Intl.DisplayNames returns country code as it is for many countries like ab: Abkhazian so I have to use fallback.

The missing languages are as follows> I think most of these are very rare and not supported by our platform anyways, so we could leave them out, but if you want to include support I think it's better to just include the following list rather than adding iso-639-1 which is a 35kB dependency.

/** Languages that may be missing from the Intl.DisplayNames API */ const obscureLanguages = { aa: 'Afar', ab: 'Abkhazian', av: 'Avaric', ae: 'Avestan', ba: 'Bashkir', bi: 'Bislama', ch: 'Chamorro', ce: 'Chechen', cu: 'Church Slavic', cv: 'Chuvash', kw: 'Cornish', cr: 'Cree', dz: 'Dzongkha', fj: 'Fijian', ff: 'Fulah', gv: 'Manx', hz: 'Herero', ho: 'Hiri Motu', io: 'Ido', ii: 'Sichuan Yi', iu: 'Inuktitut', ie: 'Interlingue', ik: 'Inupiaq', kl: 'Kalaallisut', ks: 'Kashmiri', kr: 'Kanuri', ki: 'Kikuyu', kv: 'Komi', kg: 'Kongo', kj: 'Kuanyama', li: 'Limburgan', lu: 'Luba-Katanga', mh: 'Marshallese', na: 'Nauru', nv: 'Navajo', nr: 'Ndebele: South', nd: 'Ndebele: North', ng: 'Ndonga', oj: 'Ojibwa', os: 'Ossetian', pi: 'Pali', rn: 'Rundi', sg: 'Sango', se: 'Northern Sami', sc: 'Sardinian', ss: 'Swati', ty: 'Tahitian', bo: 'Tibetan', ve: 'Venda', vo: 'Volapük', za: 'Zhuang', };

…es API openedx#2332

ahtesham-quraish · 2025-08-26T06:37:29Z

@bradenmacdonald I tried to reply on your comments can you please look into those and let me know in any case.

ahtesham-quraish force-pushed the ahtesham/#2332 branch from fd2e479 to 752cb2d Compare August 12, 2025 08:08

ahtesham-quraish force-pushed the ahtesham/#2332 branch 2 times, most recently from dd4af33 to 9f4da76 Compare August 12, 2025 08:45

ahtesham-quraish requested a review from bradenmacdonald August 12, 2025 10:57

bradenmacdonald reviewed Aug 19, 2025

View reviewed changes

fix: Replace custom language names list with standard Intl DisplayNam…

e235984

…es API openedx#2332

ahtesham-quraish force-pushed the ahtesham/#2332 branch from 9f4da76 to e235984 Compare August 26, 2025 06:36

	"test": "TZ=UTC fedx-scripts jest --passWithNoTests",
	"test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",

		import { FileInput, fileInput } from '../../../../../../sharedComponents/FileInput';
		import { getLanguageName } from '../../../../../../data/constants/video';

	import { getLanguageName } from '../../../../../../data/constants/video';
	import { getLanguageName } from '@src/editors/data/constants/video';

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

Are you sure you want to change the base?

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

Uh oh!

Conversation

ahtesham-quraish commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Best Practices Checklist

Uh oh!

codecov bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahtesham-quraish commented Aug 26, 2025

Uh oh!

Uh oh!

ahtesham-quraish commented Aug 12, 2025 •

edited

Loading

codecov bot commented Aug 12, 2025 •

edited

Loading