Unify creatExpression and createExpressionWithErrorHandling

Closes #5409

bench/benchmarks/expressions.js

@@ -7,7 +7,7 @@ const createFunction = require('../../src/style-spec/function');
 const convertFunction = require('../../src/style-spec/function/convert');
 const {
     isExpression,
-    createExpressionWithErrorHandling,
+    createExpression,
     getExpectedType,
     getDefaultValue
 } = require('../../src/style-spec/expression');
@@ -40,11 +40,14 @@ class ExpressionBenchmark extends Benchmark {
                     const expressionData = function(rawValue, propertySpec: StylePropertySpecification) {
                         const rawExpression = convertFunction(rawValue, propertySpec);
                         const compiledFunction = createFunction(rawValue, propertySpec);
-                        const compiledExpression = createExpressionWithErrorHandling(rawExpression, {
+                        const compiledExpression = createExpression(rawExpression, {
                             context: 'declaration',
                             expectedType: getExpectedType(propertySpec),
                             defaultValue: getDefaultValue(propertySpec)
                         });
+                        if (compiledExpression.result !== 'success') {
+                            throw new Error(compiledExpression.errors.map(err => `${err.key}: ${err.message}`).join(', '));
+                        }
                         return {
                             propertySpec,
                             rawValue,
@@ -97,7 +100,7 @@ class FunctionConvert extends ExpressionBenchmark {
 class ExpressionCreate extends ExpressionBenchmark {
     bench() {
         for (const {rawExpression, propertySpec} of this.data) {
-            createExpressionWithErrorHandling(rawExpression, {
+            createExpression(rawExpression, {
                 context: 'declaration',
                 expectedType: getExpectedType(propertySpec),
                 defaultValue: getDefaultValue(propertySpec)

src/style-spec/expression/index.js

@@ -11,7 +11,6 @@ const Let = require('./definitions/let');
 const definitions = require('./definitions');
 const isConstant = require('./is_constant');
 const {unwrap} = require('./values');
-const extend = require('../util/extend');
 
 import type {Type} from './types';
 import type {Value} from './values';
@@ -76,6 +75,15 @@ export type StyleExpression = StyleDeclarationExpression | StyleFilterExpression
 type StylePropertyValue = null | string | number | Array<string> | Array<number>;
 type FunctionParameters = DataDrivenPropertyValueSpecification<StylePropertyValue>
 
+/**
+ * Parse and typecheck the given style spec JSON expression.  If
+ * options.defaultValue is provided, then the resulting StyleExpression's
+ * `evaluate()` method will handle errors by logging a warning (once per
+ * message) and returning the default value.  Otherwise, it will throw
+ * evaluation errors.
+ *
+ * @private
+ */
 function createExpression(expression: mixed, options: StyleExpressionOptions): StyleExpressionErrors | StyleExpression {
     const parser = new ParsingContext(definitions, [], options.expectedType);
     const parsed = parser.parse(expression);
@@ -88,10 +96,36 @@ function createExpression(expression: mixed, options: StyleExpressionOptions): S
     }
 
     const evaluator = new EvaluationContext();
-    function evaluate(globals, feature) {
-        evaluator.globals = globals;
-        evaluator.feature = feature;
-        return parsed.evaluate(evaluator);
+
+    let evaluate;
+    if (options.defaultValue === undefined) {
+        evaluate = function (globals, feature) {
+            evaluator.globals = globals;
+            evaluator.feature = feature;
+            return parsed.evaluate(evaluator);
+        };
+    } else {
+        const warningHistory: {[key: string]: boolean} = {};
+        const defaultValue = options.defaultValue;
+        evaluate = function (globals, feature) {
+            evaluator.globals = globals;
+            evaluator.feature = feature;
+            try {
+                const val = parsed.evaluate(evaluator);
+                if (val === null || val === undefined) {
+                    return unwrap(defaultValue);
+                }
+                return unwrap(val);
+            } catch (e) {
+                if (!warningHistory[e.message]) {
+                    warningHistory[e.message] = true;
+                    if (typeof console !== 'undefined') {
+                        console.warn(e.message);
+                    }
+                }
+                return unwrap(defaultValue);
+            }
+        };
     }
 
     const isFeatureConstant = isConstant.isFeatureConstant(parsed);
@@ -146,41 +180,7 @@ function createExpression(expression: mixed, options: StyleExpressionOptions): S
     };
 }
 
-function createExpressionWithErrorHandling(expression: mixed, options: StyleExpressionOptions): StyleExpression {
-    expression = createExpression(expression, options);
-    const defaultValue = options.defaultValue === undefined ? null : options.defaultValue;
-
-    if (expression.result !== 'success') {
-        // this should have been caught in validation
-        throw new Error(expression.errors.map(err => `${err.key}: ${err.message}`).join(', '));
-    }
-
-    const evaluate = expression.evaluate;
-    const warningHistory: {[key: string]: boolean} = {};
-
-    return extend({}, expression, {
-        evaluate(globals, feature) {
-            try {
-                const val = evaluate(globals, feature);
-                if (val === null || val === undefined) {
-                    return unwrap(defaultValue);
-                }
-                return unwrap(val);
-            } catch (e) {
-                if (!warningHistory[e.message]) {
-                    warningHistory[e.message] = true;
-                    if (typeof console !== 'undefined') {
-                        console.warn(e.message);
-                    }
-                }
-                return unwrap(defaultValue);
-            }
-        }
-    });
-}
-
-module.exports = createExpression;
-module.exports.createExpressionWithErrorHandling = createExpressionWithErrorHandling;
+module.exports.createExpression = createExpression;
 module.exports.isExpression = isExpression;
 module.exports.getExpectedType = getExpectedType;
 module.exports.getDefaultValue = getDefaultValue;

src/style-spec/validate/validate_expression.js

@@ -1,6 +1,6 @@
 
 const ValidationError = require('../error/validation_error');
-const createExpression = require('../expression');
+const {createExpression} = require('../expression');
 const {getExpectedType, getDefaultValue} = require('../expression');
 const unbundle = require('../util/unbundle_jsonlint');
 

src/style/style_declaration.js

@@ -1,7 +1,7 @@
 // @flow
 
 const parseColor = require('../style-spec/util/parse_color');
-const {createExpressionWithErrorHandling, getExpectedType, getDefaultValue} = require('../style-spec/expression');
+const {createExpression, getExpectedType, getDefaultValue} = require('../style-spec/expression');
 const createFunction = require('../style-spec/function');
 const util = require('../util/util');
 const Curve = require('../style-spec/expression/definitions/curve');
@@ -31,13 +31,18 @@ function normalizeToExpression(parameters, propertySpec, name): StyleDeclaration
     }
 
     if (parameters.expression) {
-        const expression = createExpressionWithErrorHandling(
+        const expression = createExpression(
             parameters.expression, {
                 context: 'declaration',
                 expectedType: getExpectedType(propertySpec),
                 defaultValue: getDefaultValue(propertySpec)
             });
 
+        if (expression.result !== 'success') {
+            // this should have been caught in validation
+            throw new Error(expression.errors.map(err => `${err.key}: ${err.message}`).join(', '));
+        }
+
         if (expression.context === 'declaration') {
             return expression;
         } else {

test/expression.test.js

@@ -2,7 +2,7 @@
 
 require('flow-remove-types/register');
 const expressionSuite = require('./integration').expression;
-const createExpression = require('../src/style-spec/expression');
+const { createExpression } = require('../src/style-spec/expression');
 const { toString } = require('../src/style-spec/expression/types');
 const { unwrap } = require('../src/style-spec/expression/values');
 
@@ -16,8 +16,7 @@ expressionSuite.run('js', {tests: tests}, (fixture) => {
     const expression = createExpression(
         fixture.expression, {
             context: 'declaration',
-            expectedType: fixture.expectExpressionType || null,
-            defaultValue: null
+            expectedType: fixture.expectExpressionType || null
         });
 
     if (expression.result === 'error') {