refactor: simplify scale range parameters and tests (#5145)

src/compile/layoutsize/init.ts

@@ -17,12 +17,14 @@ export function initLayoutSize({
     const sizeType = getSizeType(channel);
     const fieldDef = getFieldDef(encoding[channel]) as PositionFieldDef<string>;
     if (isStep(size[sizeType])) {
-      if (isContinuous(fieldDef)) {
-        delete size[sizeType];
-        log.warn(log.message.cannotUseStepWithContinuous(sizeType));
-      } else if (isDiscrete(fieldDef) && fit) {
-        delete size[sizeType];
-        log.warn(log.message.cannotUseStepWithFit(sizeType));
+      if (fieldDef) {
+        if (isContinuous(fieldDef)) {
+          delete size[sizeType];
+          log.warn(log.message.stepDropped(sizeType, 'continuous'));
+        } else if (isDiscrete(fieldDef) && fit) {
+          delete size[sizeType];
+          log.warn(log.message.stepDropped(sizeType, 'fit'));
+        }
       }
     }
   }

src/compile/scale/parse.ts

@@ -20,14 +20,16 @@ import {parseScaleDomain} from './domain';
 import {parseScaleProperty, parseScaleRange} from './properties';
 import {scaleType} from './type';
 
-export function parseScales(model: Model) {
+export function parseScales(model: Model, {ignoreRange}: {ignoreRange?: boolean} = {}) {
   parseScaleCore(model);
   parseScaleDomain(model);
   for (const prop of NON_TYPE_DOMAIN_RANGE_VEGA_SCALE_PROPERTIES) {
     parseScaleProperty(model, prop);
   }
-  // range depends on zero
-  parseScaleRange(model);
+  if (!ignoreRange) {
+    // range depends on zero
+    parseScaleRange(model);
+  }
 }
 
 export function parseScaleCore(model: Model) {

src/compile/scale/range.ts

@@ -28,16 +28,14 @@ import {
   isContinuousToDiscrete,
   isExtendedScheme,
   Scale,
-  ScaleType,
   scaleTypeSupportProperty,
   Scheme
 } from '../../scale';
 import {isStep, LayoutSizeMixins} from '../../spec/base';
-import {Type} from '../../type';
 import * as util from '../../util';
 import {isSignalRef, SchemeConfig, VgRange} from '../../vega.schema';
 import {getBinSignalName} from '../data/bin';
-import {Rename, SignalRefWrapper} from '../signal';
+import {SignalRefWrapper} from '../signal';
 import {Explicit, makeExplicit, makeImplicit} from '../split';
 import {UnitModel} from '../unit';
 import {ScaleComponentIndex} from './component';
@@ -57,31 +55,8 @@ export function parseUnitScaleRange(model: UnitModel) {
     if (!localScaleCmpt) {
       return;
     }
-    const mergedScaleCmpt = model.getScaleComponent(channel);
 
-    const specifiedScale = model.specifiedScales[channel];
-    const fieldDef = model.fieldDef(channel);
-
-    const scaleType = mergedScaleCmpt.get('type');
-    const sizeType = getSizeType(channel);
-
-    const rangeWithExplicit = parseRangeForChannel(
-      channel,
-      model.getSignalName.bind(model),
-      scaleType,
-      fieldDef.type,
-      specifiedScale,
-      model.config,
-      localScaleCmpt.get('zero'),
-      model.mark,
-      model.getName(sizeType),
-      model.size,
-      model.fit,
-      {
-        x: getBinStepSignal(model, 'x'),
-        y: getBinStepSignal(model, 'y')
-      }
-    );
+    const rangeWithExplicit = parseRangeForChannel(channel, model);
 
     localScaleCmpt.setWithExplicit('range', rangeWithExplicit);
   });
@@ -108,20 +83,13 @@ function getBinStepSignal(model: UnitModel, channel: 'x' | 'y'): SignalRefWrappe
 /**
  * Return mixins that includes one of the Vega range types (explicit range, range.step, range.scheme).
  */
-export function parseRangeForChannel(
-  channel: Channel,
-  getSignalName: Rename,
-  scaleType: ScaleType,
-  type: Type,
-  specifiedScale: Scale,
-  config: Config,
-  zero: boolean,
-  mark: Mark,
-  sizeSignal: string,
-  size: LayoutSizeMixins,
-  fit: boolean = false,
-  xyStepSignals: {x?: SignalRefWrapper; y?: SignalRefWrapper} = {}
-): Explicit<VgRange> {
+export function parseRangeForChannel(channel: ScaleChannel, model: UnitModel): Explicit<VgRange> {
+  const specifiedScale = model.specifiedScales[channel];
+  const {size, fit} = model;
+
+  const mergedScaleCmpt = model.getScaleComponent(channel);
+  const scaleType = mergedScaleCmpt.get('type');
+
   // Check if any of the range properties is specified.
   // If so, check if it is compatible and make sure that we only output one of the properties
   for (const property of RANGE_PROPERTIES) {
@@ -161,22 +129,7 @@ export function parseRangeForChannel(
     }
   }
 
-  return makeImplicit(
-    defaultRange(
-      channel,
-      getSignalName,
-      scaleType,
-      type,
-      config,
-      zero,
-      mark,
-      sizeSignal,
-      size,
-      xyStepSignals,
-      specifiedScale.domain,
-      fit
-    )
-  );
+  return makeImplicit(defaultRange(channel, model));
 }
 
 function parseScheme(scheme: Scheme): SchemeConfig {
@@ -189,23 +142,21 @@ function parseScheme(scheme: Scheme): SchemeConfig {
   return {scheme: scheme};
 }
 
-function defaultRange(
-  channel: Channel,
-  getSignalName: Rename,
-  scaleType: ScaleType,
-  type: Type,
-  config: Config,
-  zero: boolean,
-  mark: Mark,
-  sizeSignal: string,
-  size: LayoutSizeMixins,
-  xyStepSignals: {x?: SignalRefWrapper; y?: SignalRefWrapper},
-  domain: Domain,
-  fit: boolean
-): VgRange {
+function defaultRange(channel: ScaleChannel, model: UnitModel): VgRange {
+  const {size, config, fit, mark} = model;
+
+  const getSignalName = model.getSignalName.bind(model);
+
+  const {type} = model.fieldDef(channel);
+
+  const mergedScaleCmpt = model.getScaleComponent(channel);
+  const scaleType = mergedScaleCmpt.get('type');
+
+  const {domain} = model.specifiedScales[channel];
+
   switch (channel) {
     case X:
-    case Y:
+    case Y: {
       // If there is no explicit width/height for discrete x/y scales
       if (util.contains(['point', 'band'], scaleType)) {
         if (channel === X && !size.width) {
@@ -228,16 +179,21 @@ function defaultRange(
       // We will later replace these temporary names with
       // the final name in assembleScaleRange()
 
+      const sizeType = getSizeType(channel);
+      const sizeSignal = model.getName(sizeType);
+
       if (channel === Y && hasContinuousDomain(scaleType)) {
         // For y continuous scale, we have to start from the height as the bottom part has the max value.
         return [SignalRefWrapper.fromName(getSignalName, sizeSignal), 0];
       } else {
         return [0, SignalRefWrapper.fromName(getSignalName, sizeSignal)];
       }
+    }
     case SIZE: {
       // TODO: support custom rangeMin, rangeMax
+      const zero = model.component.scales[channel].get('zero');
       const rangeMin = sizeRangeMin(mark, zero, config);
-      const rangeMax = sizeRangeMax(mark, size, xyStepSignals, config);
+      const rangeMax = sizeRangeMax(mark, size, model, config);
       if (isContinuousToDiscrete(scaleType)) {
         return interpolateRange(
           rangeMin,
@@ -341,12 +297,12 @@ function sizeRangeMin(mark: Mark, zero: boolean, config: Config) {
 
 export const MAX_SIZE_RANGE_STEP_RATIO = 0.95;
 
-function sizeRangeMax(
-  mark: Mark,
-  size: LayoutSizeMixins,
-  xyStepSignals: {x?: SignalRefWrapper; y?: SignalRefWrapper},
-  config: Config
-): number | SignalRef {
+function sizeRangeMax(mark: Mark, size: LayoutSizeMixins, model: UnitModel, config: Config): number | SignalRef {
+  const xyStepSignals = {
+    x: getBinStepSignal(model, 'x'),
+    y: getBinStepSignal(model, 'y')
+  };
+
   switch (mark) {
     case 'bar':
     case 'tick': {

src/log/message.ts

@@ -21,14 +21,6 @@ export const INVALID_SPEC = 'Invalid spec';
 // FIT
 export const FIT_NON_SINGLE = 'Autosize "fit" only works for single views and layered views.';
 
-export function cannotUseStepWithFit(sizeType: 'width' | 'height') {
-  return `Cannot use a "step" ${sizeType} when "autosize" is "fit".`;
-}
-
-export function cannotUseStepWithContinuous(sizeType: 'width' | 'height') {
-  return `Cannot use a "step" ${sizeType} when the ${sizeType === 'width' ? 'x' : 'y'}-field is continuous.`;
-}
-
 // SELECTION
 export function cannotProjectOnChannelWithoutField(channel: Channel) {
   return `Cannot project a selection on encoding channel "${channel}", which has no field.`;

src/scale.ts

@@ -124,6 +124,8 @@ export function scaleTypePrecedence(scaleType: ScaleType): number {
 export const CONTINUOUS_TO_CONTINUOUS_SCALES: ScaleType[] = ['linear', 'log', 'pow', 'sqrt', 'symlog', 'time', 'utc'];
 const CONTINUOUS_TO_CONTINUOUS_INDEX = toSet(CONTINUOUS_TO_CONTINUOUS_SCALES);
 
+export const QUANTITATIVE_SCALES: ScaleType[] = ['linear', 'log', 'pow', 'sqrt', 'symlog'];
+
 export const CONTINUOUS_TO_DISCRETE_SCALES: ScaleType[] = ['quantile', 'quantize', 'threshold'];
 const CONTINUOUS_TO_DISCRETE_INDEX = toSet(CONTINUOUS_TO_DISCRETE_SCALES);
 

test/compile/compile.test.ts

@@ -135,7 +135,7 @@ describe('compile/compile', () => {
           x: {field: 'b', type: 'quantitative'}
         }
       }).spec;
-      expect(localLogger.warns[0]).toEqual(log.message.cannotUseStepWithFit('height'));
+      expect(localLogger.warns[0]).toEqual(log.message.stepDropped('height', 'fit'));
       expect(spec.width).toEqual(200);
       expect(spec.height).toEqual(200);
     })

test/compile/layoutsize/init.test.ts

@@ -14,7 +14,7 @@ describe('compile/layout', () => {
           }
         });
 
-        expect(localLogger.warns[0]).toEqual(log.message.cannotUseStepWithContinuous('width'));
+        expect(localLogger.warns[0]).toEqual(log.message.stepDropped('width', 'continuous'));
 
         expect(model.component.layoutSize.get('width')).toBe(200);
       })