[GitHub] [superset] villebro commented on a change in pull request #16482: fix: ensure array return value in DND selector

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #16482:
URL: https://github.com/apache/superset/pull/16482#discussion_r699103821



##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
     return !!this.getValues()?.includes(value);
   }
 
-  getValues(): string[] | string | undefined {
+  getValues(): string[] | undefined {
+    // ensure an array type return value
     if (!this.multi) {
-      return this.values.length > 0 ? this.values[0].column_name : undefined;
+      return this.values.length > 0 ? [this.values[0].column_name] : undefined;

Review comment:
       I assume the current return type `string[] | string | undefined` is there to make it fully compatible with the non-dnd control, which returns `string` when not in `multi` mode. Am I right @kgabryje?

##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx
##########
@@ -59,8 +59,9 @@ export function DndColumnSelect(props: DndColumnSelectProps) {
     }
     if (
       typeof value === 'string' &&
-      typeof optionSelectorValues === 'string' &&
-      value !== optionSelectorValues
+      Array.isArray(optionSelectorValues) &&
+      optionSelectorValues.length === 1 &&
+      value !== optionSelectorValues[0]

Review comment:
       I looked at `OptionSelector.has` - I agree that should probably be changed to something like
   
   ```typescript
     has(value: string): boolean {
       return ensureIsArray(this.getValues()).includes(value);
     }
   ```
   
   Would changing that solve this problem?

##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -67,9 +67,10 @@ export class OptionSelector {
     return !!this.getValues()?.includes(value);
   }
 
-  getValues(): string[] | string | undefined {
+  getValues(): string[] | undefined {
+    // ensure an array type return value
     if (!this.multi) {
-      return this.values.length > 0 ? this.values[0].column_name : undefined;
+      return this.values.length > 0 ? [this.values[0].column_name] : undefined;

Review comment:
       Right - I think v1 charts won't mind, but `viz.py` definitely has lots of logic that either assumes control values are either arrays or strings.




-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org