fix(Changing languages doesn't affect echarts charts): issue 30131 #31751

jpchev · 2025-01-08T12:30:03Z

SUMMARY

this is to propagate the current locale up to the echarts configuration.

fixes: #30131

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

switch the locale and verify that charts have the correct locale (for example in months name and in buttons)

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Fix Detected
	Unsafe Dynamic Module Import ▹ view	✅
	Missing Locale Fallback ▹ view	✅
	Missing locale state fallback ▹ view	✅

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.

You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.

Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.

Chat with Korbit on issues we post by tagging @korbit-ai in your reply.

Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

Check out our docs on how you can make Korbit work best for you and your team.

Customize Korbit for your organization through the Korbit Console.

Feedback and Support

Tell us what you think of Korbit

Schedule a call with our team

Email us @ support@korbit.ai

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

@@ -123,10 +127,14 @@ function Echart(
    getEchartInstance: () => chartRef.current,
  }));

+  const localeObj = useSelector((state: ExplorePageState) => state?.common?.locale);
+  const lang = require('echarts/lib/i18n/lang' + localeObj.toUpperCase()).default;


superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

pomegranited

Thank you for submitting this PR @jpchev , you beat me to it :)
With just a few small changes, this is working well for me.

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

pomegranited

👍 This change is working great! Thank you @jpchev :)

I tested this by combining it with fix(lang): patch FAB's LocaleView to redirect to previous page #31692 to test changing languages while viewing and editing the Pie Chart echart. I verified that this change ensures the "Inv" and "All" buttons on that echart are now translated as expected.
I read through the code

translate.echarts.mp4

rusackas · 2025-01-22T06:22:55Z

@villebro might be most qualified to look at this one, if he has time. That said, there are some failing tests that could use attention, and a more descritive PR title could help too :D

jpchev · 2025-01-22T12:40:31Z

@villebro might be most qualified to look at this one, if he has time. That said, there are some failing tests that could use attention, and a more descritive PR title could help too :D

sorry for the cryptic title, I've put the title of the issue it fixes.
Also, I see one of the checks fails because I used the 'require' import

https://github.com/apache/superset/actions/runs/12825802735/job/35764682648?pr=31751#step:4:4582

I know it's not recommended, but I don't know how to dynamically import a js file without using 'require'.
There is an async alternative, but that would make the code much less clear, in my opinion.

pomegranited · 2025-01-23T01:19:55Z

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

+  const locale = useSelector(
+    (state: ExplorePageState) => state?.common?.locale ?? 'en',
+  ).toUpperCase();
+  try {
+    const lang = require(`echarts/lib/i18n/lang${locale}`).default;
+    registerLocale(locale, lang);
+  } catch (e) {
+    console.warn(`Locale ${locale} not supported in ECharts`);
+  }
+


@jpchev I think we need to use dynamic imports instead, but they're tricky to use in React. I tried a few ways and wasn't able to get it to render properly, so am asking the bot:

@dosu how do we refactor this to ensure we await the import and registerLocale calls before calling init below?

Reply is here: #30131 (comment)

But it doesn't work :(

Yes, I tried something similar, without success. That's why I went for require.

@jpchev I found the culprit! handleSizeChange wasn't running because of race conditions on render.

See jpchev#2

jpchev · 2025-01-24T12:50:08Z

@pomegranited I fixed almost all the checks, there is this one left which I don't how to treat https://github.com/apache/superset/actions/runs/12949242972/job/36119814005?pr=31751

do you have an idea? It seems there are errors also on files not impacted by changes in this PR,
for instance

@superset-ui/plugin-chart-echarts: ../../scripts/tsc.sh: line 25: exit: ../../src/components/Icons/Icon.tsx(23,29): error TS2307: Cannot find module 'src/assets/images/icons/transparent.svg' or its corresponding type declarations.

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

superset-frontend/plugins/plugin-chart-echarts/package.json

pomegranited · 2025-01-29T12:27:02Z

superset-frontend/package-lock.json

@@ -50218,6 +50218,14 @@
      "version": "4.2.0",
      "license": "MIT"
    },
+    "node_modules/superset": {


Could you revert these changes? I don't think they're needed.

jpchev · 2025-01-29T13:32:17Z

@pomegranited I've pushed again my code from scratch, in order to resolve the conflict on superset-frontend/package-lock.json I think it needs again to be approved

…into fix/issue_30131

jpchev · 2025-01-29T15:29:48Z

@pomegranited all checks pass now

superset-frontend/plugins/plugin-chart-echarts/package.json

jpchev · 2025-01-31T10:29:48Z

all tests pass now,
but due to a merge conflict I had to remove all my commits and push again,
and that closed the PR, so I had to reopen again and that requires again your approval.

@villebro @michael-s-molina could you please have a look if it is ok?

Thank you

jpchev · 2025-02-07T12:50:46Z

@pomegranited do you have news? It seems this PR can be merged, all tests pass and we fixed all the detected issues

pomegranited · 2025-02-11T00:27:07Z

@jpchev I have no news -- we're just waiting for upstream to review and merge here.

villebro

Thanks for taking this up! A few comments, LMKWYT.

villebro · 2025-02-19T23:49:41Z

superset-frontend/plugins/plugin-chart-echarts/package.json

-    "lodash": "^4.17.21",
-    "dayjs": "^1.11.13"
+    "dayjs": "^1.11.13",
+    "lodash": "^4.17.21"


not your fault, but isn't lodash a peer dependency?

villebro · 2025-02-19T23:52:19Z

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

+// Define this interface here to avoid creating a dependency back to superset-frontend,
+// and to appease the compiler.
+// ref: superset-frontend/src/explore/types
+interface ExplorePageState {
+  common: {
+    locale: string;
+  };
+}


Oof.. I agree with the workaround here, but could we rephrase the comment to a // TODO: to move the type to @superset-ui/core?

villebro · 2025-02-19T23:54:25Z

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx

@@ -123,24 +134,52 @@ function Echart(
    getEchartInstance: () => chartRef.current,
  }));

+  const locale = useSelector(
+    (state: ExplorePageState) => state?.common?.locale ?? 'en',


nit: Instead of having a magic string in the codebase, it would be nice to move this to src/constants.ts as DEFAULT_LOCALE = 'en'; or similar.

rusackas · 2025-02-20T23:07:22Z

This is looking great! Re-running a failing (and probably just flaky) CI check.

github-actions · 2025-02-20T23:07:53Z

@rusackas Processing your ephemeral environment request here. Action: up.

villebro

One last comment

villebro · 2025-02-20T23:10:18Z

superset-frontend/plugins/plugin-chart-echarts/package.json

@@ -24,6 +24,7 @@
    "lib"
  ],
  "dependencies": {
+    "@types/react-redux": "^7.1.10",


shouldn't this be devDependencies?

github-actions · 2025-02-20T23:17:14Z

@rusackas Ephemeral environment spinning up at http://54.186.129.54:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

villebro · 2025-02-21T16:59:28Z

superset-frontend/plugins/plugin-chart-echarts/package.json

@@ -33,7 +33,8 @@
    "@superset-ui/core": "*",
    "echarts": "*",
    "memoize-one": "*",
-    "react": "^17.0.2"
+    "react": "^17.0.2",
+    "@types/react-redux": "^7.1.10"


I'm not sure why we'd want to have this under peerDepdendencies - let's just move it under devDependencies.

pull-request-size bot added the size/S label Jan 8, 2025

github-actions bot added the plugins label Jan 8, 2025

dosubot bot added the viz:charts:echarts Related to Echarts label Jan 8, 2025

jpchev mentioned this pull request Jan 8, 2025

Changing languages doesn't affect echarts charts. #30131

Open

3 tasks

korbit-ai bot reviewed Jan 8, 2025

View reviewed changes

jpchev changed the title ~~fixes for https://github.com/apache/superset/issues/30131~~ fix(issue): 30131 Jan 8, 2025

sadpandajoe requested review from villebro and michael-s-molina January 8, 2025 18:32

justinpark reviewed Jan 8, 2025

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx Outdated Show resolved Hide resolved

rusackas mentioned this pull request Jan 9, 2025

build(eslint): disabling wildcard imports with eslint #31761

Merged

9 tasks

pomegranited reviewed Jan 16, 2025

View reviewed changes

pomegranited approved these changes Jan 17, 2025

View reviewed changes

pomegranited mentioned this pull request Jan 21, 2025

Superset i18n: Fix language settings in echarts openedx/openedx-aspects#300

Open

1 task

jpchev changed the title ~~fix(issue): 30131~~ fix(Changing languages doesn't affect echarts charts): 30131 Jan 22, 2025

jpchev changed the title ~~fix(Changing languages doesn't affect echarts charts): 30131~~ fix(Changing languages doesn't affect echarts charts): issue 30131 Jan 22, 2025

pomegranited reviewed Jan 23, 2025

View reviewed changes

pull-request-size bot added size/M and removed size/S labels Jan 24, 2025

github-actions bot added the dependencies:npm label Jan 24, 2025

pomegranited reviewed Jan 29, 2025

View reviewed changes

jpchev force-pushed the fix/issue_30131 branch from bba5d7a to 0c8010c Compare January 29, 2025 14:03

jpchev closed this Jan 29, 2025

jpchev force-pushed the fix/issue_30131 branch from e19e66c to e4bdb28 Compare January 29, 2025 14:21

pull-request-size bot added size/XS and removed size/M labels Jan 29, 2025

changes

8a9f4dd

jpchev reopened this Jan 29, 2025

pull-request-size bot added size/M and removed size/XS labels Jan 29, 2025

jpchev and others added 3 commits January 29, 2025 15:44

fixed react version in plugin-char-echarts/package.json

79a6235

prettier

50a6ab0

Merge branch 'fix/issue_30131' of https://github.com/jpchev/superset …

047511f

…into fix/issue_30131

pomegranited reviewed Jan 30, 2025

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/package.json Outdated Show resolved Hide resolved

jpchev added 2 commits January 30, 2025 09:33

Merge branch 'apache:master' into fix/issue_30131

f0ef041

Update package.json

543598a

rusackas requested a review from kgabryje February 19, 2025 23:34

villebro reviewed Feb 19, 2025

View reviewed changes

jpchev added 5 commits February 20, 2025 17:38

Update Echart.tsx

859fcc8

Update constants.ts

12aaee5

Update constants.ts

1f0977a

Update Echart.tsx

214f3cf

Update package.json

d472d0e

rusackas added the testenv-up label Feb 20, 2025

villebro reviewed Feb 20, 2025

View reviewed changes

jpchev added 2 commits February 21, 2025 09:40

Merge branch 'apache:master' into fix/issue_30131

7637531

Update package.json

1350795

villebro reviewed Feb 21, 2025

View reviewed changes

Update package.json

92cad79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Changing languages doesn't affect echarts charts): issue 30131 #31751

fix(Changing languages doesn't affect echarts charts): issue 30131 #31751

jpchev commented Jan 8, 2025 •

edited by sadpandajoe

Loading

korbit-ai bot left a comment •

edited

Loading

This comment was marked as resolved.

pomegranited left a comment

pomegranited left a comment •

edited

Loading

rusackas commented Jan 22, 2025 •

edited

Loading

jpchev commented Jan 22, 2025 •

edited

Loading

pomegranited Jan 23, 2025

pomegranited Jan 23, 2025

jpchev Jan 23, 2025

pomegranited Jan 24, 2025

jpchev commented Jan 24, 2025

pomegranited Jan 29, 2025

jpchev commented Jan 29, 2025 •

edited

Loading

jpchev commented Jan 29, 2025 •

edited

Loading

jpchev commented Jan 31, 2025 •

edited

Loading

jpchev commented Feb 7, 2025

pomegranited commented Feb 11, 2025

villebro left a comment

villebro Feb 19, 2025

villebro Feb 19, 2025

villebro Feb 19, 2025

rusackas commented Feb 20, 2025

github-actions bot commented Feb 20, 2025

villebro left a comment

villebro Feb 20, 2025

github-actions bot commented Feb 20, 2025

villebro Feb 21, 2025

fix(Changing languages doesn't affect echarts charts): issue 30131 #31751

Are you sure you want to change the base?

fix(Changing languages doesn't affect echarts charts): issue 30131 #31751

Conversation

jpchev commented Jan 8, 2025 • edited by sadpandajoe Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

korbit-ai bot left a comment • edited Loading

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Interacting with Korbit

Customizing Korbit

Feedback and Support

This comment was marked as resolved.

pomegranited left a comment

Choose a reason for hiding this comment

pomegranited left a comment • edited Loading

Choose a reason for hiding this comment

rusackas commented Jan 22, 2025 • edited Loading

jpchev commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpchev commented Jan 24, 2025

Choose a reason for hiding this comment

jpchev commented Jan 29, 2025 • edited Loading

jpchev commented Jan 29, 2025 • edited Loading

jpchev commented Jan 31, 2025 • edited Loading

jpchev commented Feb 7, 2025

pomegranited commented Feb 11, 2025

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas commented Feb 20, 2025

github-actions bot commented Feb 20, 2025

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2025

Choose a reason for hiding this comment

jpchev commented Jan 8, 2025 •

edited by sadpandajoe

Loading

korbit-ai bot left a comment •

edited

Loading

pomegranited left a comment •

edited

Loading

rusackas commented Jan 22, 2025 •

edited

Loading

jpchev commented Jan 22, 2025 •

edited

Loading

jpchev commented Jan 29, 2025 •

edited

Loading

jpchev commented Jan 29, 2025 •

edited

Loading

jpchev commented Jan 31, 2025 •

edited

Loading