Re: [PR] fix(echart): Thrown errors shown after resized [superset]

via GitHub Tue, 15 Apr 2025 18:49:15 -0700


korbit-ai[bot] commented on code in PR #33143:
URL: https://github.com/apache/superset/pull/33143#discussion_r2045849494



##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -106,6 +107,15 @@ use([
   LabelLayout,
 ]);
 
+const loadLocale = async (locale: string) => {
+  try {
+    const lang = await import(`echarts/lib/i18n/lang${locale}`);
+    return lang?.default;
+  } catch (e) {
+    console.error(`Locale ${locale} not supported in ECharts`, e);

Review Comment:
   ### Unstructured console.error usage <sub>![category 
Logging](https://img.shields.io/badge/Logging-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Using console.error for error logging instead of a proper logging framework. 
Also, the error object 'e' is directly passed without structure.
   
   ###### Why this matters
   Console.error logs are not captured by logging infrastructure, making it 
difficult to monitor and track locale loading failures in production 
environments.
   
   ###### Suggested change ∙ *Feature Preview*
   ```typescript
   const loadLocale = async (locale: string) => {
     try {
       const lang = await import(`echarts/lib/i18n/lang${locale}`);
       return lang?.default;
     } catch (error) {
       logger.error('Failed to load ECharts locale', {
         locale,
         error: {
           message: error.message,
           stack: error.stack,
         },
         component: 'Echart'
       });
     }
   };
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd4ac080-169a-4528-b2cc-b06c68f6c4c8/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd4ac080-169a-4528-b2cc-b06c68f6c4c8?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd4ac080-169a-4528-b2cc-b06c68f6c4c8?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd4ac080-169a-4528-b2cc-b06c68f6c4c8?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fd4ac080-169a-4528-b2cc-b06c68f6c4c8)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:bfba0f39-7687-4567-a237-fca2d1bfbfa6 -->
   
   
   [](bfba0f39-7687-4567-a237-fca2d1bfbfa6)



##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -106,6 +107,15 @@
   LabelLayout,
 ]);
 
+const loadLocale = async (locale: string) => {
+  try {
+    const lang = await import(`echarts/lib/i18n/lang${locale}`);
+    return lang?.default;
+  } catch (e) {
+    console.error(`Locale ${locale} not supported in ECharts`, e);
+  }
+};

Review Comment:
   ### Tightly coupled locale loading <sub>![category 
Design](https://img.shields.io/badge/Design-0d9488)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The locale loading logic is tightly coupled with the ECharts implementation 
and error handling is incomplete.
   
   ###### Why this matters
   Makes it difficult to change locale loading strategy or handle errors 
properly at the application level. The function also doesn't follow the 
Interface Segregation Principle by mixing concerns.
   
   ###### Suggested change ∙ *Feature Preview*
   Create a separate locale service with proper error handling:
   ```typescript
   interface LocaleLoader {
     loadLocale(locale: string): Promise<any>;
   }
   
   class EchartsLocaleLoader implements LocaleLoader {
     async loadLocale(locale: string): Promise<any> {
       try {
         const lang = await import(`echarts/lib/i18n/lang${locale}`);
         return lang?.default;
       } catch (error) {
         throw new LocaleLoadError(locale, error);
       }
     }
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/70239cc4-1c45-4feb-9d71-25f3b33a71ba/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/70239cc4-1c45-4feb-9d71-25f3b33a71ba?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/70239cc4-1c45-4feb-9d71-25f3b33a71ba?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/70239cc4-1c45-4feb-9d71-25f3b33a71ba?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/70239cc4-1c45-4feb-9d71-25f3b33a71ba)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:988999b9-db70-4a45-9956-9dee7f269704 -->
   
   
   [](988999b9-db70-4a45-9956-9dee7f269704)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] fix(echart): Thrown errors shown after resized [superset]

Reply via email to