feat(encodable): only set scale domain if both bounds are defined (#250)

* feat: only set scale domain if both bounds are presented * fix: unit test * fix: more optimize
apache · Nov 26, 2021 · b672373 · b672373
1 parent f5f876f
commit b672373
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 12 deletions.
diff --git a/...y_superset_ui/superset-ui/packages/superset-ui-encodable/src/parsers/scale/applyDomain.ts b/...y_superset_ui/superset-ui/packages/superset-ui-encodable/src/parsers/scale/applyDomain.ts
@@ -1,21 +1,25 @@
+import { isDateTime } from 'vega-lite/build/src/datetime';
 import { Value } from '../../types/VegaLite';
-import { ScaleConfig, D3Scale, TimeScaleConfig } from '../../types/Scale';
+import { ScaleConfig, D3Scale } from '../../types/Scale';
 import parseDateTime from '../parseDateTime';
 import inferElementTypeFromUnionOfArrayTypes from '../../utils/inferElementTypeFromUnionOfArrayTypes';
-import { isTimeScale } from '../../typeGuards/Scale';
+import { isEveryElementDefined } from '../../typeGuards/Base';
 
 export default function applyDomain<Output extends Value>(
   config: ScaleConfig<Output>,
   scale: D3Scale<Output>,
 ) {
-  const { domain, reverse, type } = config;
-  if (typeof domain !== 'undefined') {
-    const processedDomain = reverse ? domain.slice().reverse() : domain;
-    if (isTimeScale(scale, type)) {
-      const timeDomain = processedDomain as TimeScaleConfig['domain'];
-      scale.domain(inferElementTypeFromUnionOfArrayTypes(timeDomain).map(d => parseDateTime(d)));
-    } else {
-      scale.domain(processedDomain);
+  const { domain, reverse } = config;
+  if (typeof domain !== 'undefined' && domain.length > 0) {
+    const processedDomain = inferElementTypeFromUnionOfArrayTypes(domain);
+
+    // Only set domain if all items are defined
+    if (isEveryElementDefined(processedDomain)) {
+      scale.domain(
+        (reverse ? processedDomain.slice().reverse() : processedDomain).map(d =>
+          isDateTime(d) ? parseDateTime(d) : d,
+        ),
+      );
     }
   }
 }
diff --git a/...d/temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/typeGuards/Base.ts b/...d/temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/typeGuards/Base.ts
@@ -9,3 +9,7 @@ export function isNotArray<T>(maybeArray: T | T[]): maybeArray is T {
 export function isDefined<T>(value: any): value is T {
   return typeof value !== 'undefined' && value !== null;
 }
+
+export function isEveryElementDefined<T>(array: T[]): array is Exclude<T, undefined | null>[] {
+  return array.every(isDefined);
+}
diff --git a/.../temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/typeGuards/Scale.ts b/.../temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/typeGuards/Scale.ts
@@ -10,6 +10,7 @@ import {
   SymlogScaleConfig,
   TimeScaleConfig,
   UtcScaleConfig,
+  ContinuousD3Scale,
 } from '../types/Scale';
 import { Value, ScaleType } from '../types/VegaLite';
 import { timeScaleTypesSet, continuousScaleTypesSet } from '../parsers/scale/scaleCategories';
@@ -50,6 +51,13 @@ export function isD3Scale<Output extends Value = Value>(
   return !isCategoricalColorScale(scale);
 }
 
+export function isContinuousScale<Output extends Value = Value>(
+  scale: D3Scale<Output> | CategoricalColorScale,
+  scaleType: ScaleType,
+): scale is ContinuousD3Scale<Output> {
+  return scale && continuousScaleTypesSet.has(scaleType);
+}
+
 export function isTimeScale<Output extends Value = Value>(
   scale: D3Scale<Output> | CategoricalColorScale,
   scaleType: ScaleType,

diff --git a/...ntend/temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/types/Scale.ts b/...ntend/temporary_superset_ui/superset-ui/packages/superset-ui-encodable/src/types/Scale.ts
@@ -40,7 +40,7 @@ export interface CombinedScaleConfig<Output extends Value = Value>
   /**
    * domain of the scale
    */
-  domain?: number[] | string[] | boolean[] | DateTime[];
+  domain?: (number | undefined | null)[] | string[] | boolean[] | (DateTime | undefined | null)[];
   /**
    * range of the scale
    */

diff --git a/...t-ui/packages/superset-ui-encodable/test/parsers/scale/createScaleFromScaleConfig.test.ts b/...t-ui/packages/superset-ui-encodable/test/parsers/scale/createScaleFromScaleConfig.test.ts
@@ -34,6 +34,60 @@ describe('createScaleFromScaleConfig(config)', () => {
       });
       expect(scale(10)).toEqual(0);
     });
+    describe('does not set domain if domain has undefined or null', () => {
+      describe('undefined', () => {
+        it('min', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [undefined, 30],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+        it('max', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [0, undefined],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+        it('both', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [undefined, undefined],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+      });
+      describe('null', () => {
+        it('min', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [null, 30],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+        it('max', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [0, null],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+        it('both', () => {
+          const scale = createScaleFromScaleConfig({
+            type: 'linear',
+            domain: [null, null],
+            range: [0, 100],
+          });
+          expect(scale(10)).toEqual(1000);
+        });
+      });
+    });
     it('with color scheme as range', () => {
       getSequentialSchemeRegistry().registerValue(
         'test-scheme',

diff --git a/...orary_superset_ui/superset-ui/packages/superset-ui-encodable/test/typeGuards/Base.test.ts b/...orary_superset_ui/superset-ui/packages/superset-ui-encodable/test/typeGuards/Base.test.ts
@@ -1,4 +1,4 @@
-import { isDefined, isArray, isNotArray } from '../../src/typeGuards/Base';
+import { isDefined, isArray, isNotArray, isEveryElementDefined } from '../../src/typeGuards/Base';
 
 describe('type guards: Base', () => {
   describe('isArray<T>(maybeArray)', () => {
@@ -37,4 +37,18 @@ describe('type guards: Base', () => {
       expect(isDefined(undefined)).toBeFalsy();
     });
   });
+  describe('isEveryElementDefined<T>(array)', () => {
+    it('returns true and remove undefined from possible return type', () => {
+      expect(isEveryElementDefined(['a', 'b'])).toBeTruthy();
+      expect(isEveryElementDefined([])).toBeTruthy();
+      const array: (string | undefined)[] = ['a', 'b'];
+      if (isEveryElementDefined(array)) {
+        expect(array.every(a => a.length === 1)).toBeTruthy();
+      }
+    });
+    it('returns false otherwise', () => {
+      expect(isEveryElementDefined([undefined])).toBeFalsy();
+      expect(isEveryElementDefined([undefined, 'a'])).toBeFalsy();
+    });
+  });
 });
diff --git a/...rary_superset_ui/superset-ui/packages/superset-ui-encodable/test/typeGuards/Scale.test.ts b/...rary_superset_ui/superset-ui/packages/superset-ui-encodable/test/typeGuards/Scale.test.ts
@@ -6,6 +6,7 @@ import {
   isTimeScale,
   isContinuousScaleConfig,
   isScaleConfigWithZero,
+  isContinuousScale,
 } from '../../src/typeGuards/Scale';
 import { HasToString } from '../../src/types/Base';
 
@@ -43,6 +44,14 @@ describe('type guards', () => {
       expect(isCategoricalColorScale(scaleLinear())).toBeFalsy();
     });
   });
+  describe('isContinuousScale(scale, type)', () => {
+    it('returns true if type is one of the time scale types', () => {
+      expect(isContinuousScale(scaleLinear(), 'linear')).toBeTruthy();
+    });
+    it('returns false otherwise', () => {
+      expect(isContinuousScale(scaleOrdinal<HasToString, string>(), 'ordinal')).toBeFalsy();
+    });
+  });
   describe('isTimeScale(scale, type)', () => {
     it('returns true if type is one of the time scale types', () => {
       expect(isTimeScale(scaleTime(), 'time')).toBeTruthy();