Merge pull request #473 from sveltejs/gh-166

More helpful validation

src/utils/namespaces.js

@@ -5,4 +5,9 @@ export const xlink  = 'http://www.w3.org/1999/xlink';
 export const xml    = 'http://www.w3.org/XML/1998/namespace';
 export const xmlns  = 'http://www.w3.org/2000/xmlns';
 
+export const validNamespaces = [
+	'html', 'mathml', 'svg', 'xlink', 'xml', 'xmlns',
+	html, mathml, svg, xlink, xml, xmlns
+];
+
 export default { html, mathml, svg, xlink, xml, xmlns };

src/validate/html/index.js

@@ -1,7 +1,13 @@
 import * as namespaces from '../../utils/namespaces.js';
+import flattenReference from '../../utils/flattenReference.js';
 
 const svg = /^(?:altGlyph|altGlyphDef|altGlyphItem|animate|animateColor|animateMotion|animateTransform|circle|clipPath|color-profile|cursor|defs|desc|discard|ellipse|feBlend|feColorMatrix|feComponentTransfer|feComposite|feConvolveMatrix|feDiffuseLighting|feDisplacementMap|feDistantLight|feDropShadow|feFlood|feFuncA|feFuncB|feFuncG|feFuncR|feGaussianBlur|feImage|feMerge|feMergeNode|feMorphology|feOffset|fePointLight|feSpecularLighting|feSpotLight|feTile|feTurbulence|filter|font|font-face|font-face-format|font-face-name|font-face-src|font-face-uri|foreignObject|g|glyph|glyphRef|hatch|hatchpath|hkern|image|line|linearGradient|marker|mask|mesh|meshgradient|meshpatch|meshrow|metadata|missing-glyph|mpath|path|pattern|polygon|polyline|radialGradient|rect|set|solidcolor|stop|switch|symbol|text|textPath|title|tref|tspan|unknown|use|view|vkern)$/;
 
+function list ( items, conjunction = 'or' ) {
+	if ( items.length === 1 ) return items[0];
+	return `${items.slice( 0, -1 ).join( ', ' )} ${conjunction} ${items[ items.length - 1 ]}`;
+}
+
 export default function validateHtml ( validator, html ) {
 	let elementDepth = 0;
 
@@ -12,13 +18,37 @@ export default function validateHtml ( validator, html ) {
 			}
 
 			elementDepth += 1;
+
+			node.attributes.forEach( attribute => {
+				if ( attribute.type === 'EventHandler' ) {
+					const { callee, start, type } = attribute.expression;
+
+					if ( type !== 'CallExpression' ) {
+						validator.error( `Expected a call expression`, start );
+					}
+
+					const { name } = flattenReference( callee );
+
+					if ( name === 'this' || name === 'event' ) return;
+					if ( callee.type === 'Identifier' && callee.name === 'set' || callee.name === 'fire' || callee.name in validator.methods ) return;
+
+					const validCallees = list( [ 'this.*', 'event.*', 'set', 'fire' ].concat( Object.keys( validator.methods ) ) );
+					let message = `'${validator.source.slice( callee.start, callee.end )}' is an invalid callee (should be one of ${validCallees})`;
+
+					if ( callee.type === 'Identifier' && callee.name in validator.helpers ) {
+						message += `. '${callee.name}' exists on 'helpers', did you put it in the wrong place?`;
+					}
+
+					validator.error( message, start );
+				}
+			});
 		}
 
 		if ( node.children ) {
 			node.children.forEach( visit );
 		}
 
-		if (node.else ) {
+		if ( node.else ) {
 			visit( node.else );
 		}
 

src/validate/index.js

@@ -18,7 +18,7 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file
 
 			error.toString = () => `${error.message} (${error.loc.line}:${error.loc.column})\n${error.frame}`;
 
-			onerror( error );
+			throw error;
 		},
 
 		warn: ( message, pos ) => {
@@ -36,28 +36,42 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file
 			});
 		},
 
-		namespace: null
+		source,
+
+		namespace: null,
+		defaultExport: null,
+		properties: {},
+		methods: {},
+		helpers: {}
 	};
 
-	if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) {
-		const error = new Error( `options.name must be a valid identifier` );
-		onerror( error );
-	}
+	try {
+		if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) {
+			const error = new Error( `options.name must be a valid identifier` );
+			throw error;
+		}
 
-	if ( name && !/^[A-Z]/.test( name ) ) {
-		const message = `options.name should be capitalised`;
-		onwarn({
-			message,
-			filename,
-			toString: () => message
-		});
-	}
+		if ( name && !/^[A-Z]/.test( name ) ) {
+			const message = `options.name should be capitalised`;
+			onwarn({
+				message,
+				filename,
+				toString: () => message
+			});
+		}
 
-	if ( parsed.js ) {
-		validateJs( validator, parsed.js );
-	}
+		if ( parsed.js ) {
+			validateJs( validator, parsed.js );
+		}
 
-	if ( parsed.html ) {
-		validateHtml( validator, parsed.html );
+		if ( parsed.html ) {
+			validateHtml( validator, parsed.html );
+		}
+	} catch ( err ) {
+		if ( onerror ) {
+			onerror( err );
+		} else {
+			throw err;
+		}
 	}
 }

src/validate/js/index.js

@@ -23,19 +23,19 @@ export default function validateJs ( validator, js ) {
 			checkForComputedKeys( validator, node.declaration.properties );
 			checkForDupes( validator, node.declaration.properties );
 
-			const templateProperties = {};
+			const props = validator.properties;
 
 			node.declaration.properties.forEach( prop => {
-				templateProperties[ prop.key.name ] = prop;
+				props[ prop.key.name ] = prop;
 			});
 
 			// Remove these checks in version 2
-			if ( templateProperties.oncreate && templateProperties.onrender ) {
-				validator.error( 'Cannot have both oncreate and onrender', templateProperties.onrender.start );
+			if ( props.oncreate && props.onrender ) {
+				validator.error( 'Cannot have both oncreate and onrender', props.onrender.start );
 			}
 
-			if ( templateProperties.ondestroy && templateProperties.onteardown ) {
-				validator.error( 'Cannot have both ondestroy and onteardown', templateProperties.onteardown.start );
+			if ( props.ondestroy && props.onteardown ) {
+				validator.error( 'Cannot have both ondestroy and onteardown', props.onteardown.start );
 			}
 
 			// ensure all exported props are valid
@@ -56,8 +56,8 @@ export default function validateJs ( validator, js ) {
 				}
 			});
 
-			if ( templateProperties.namespace ) {
-				const ns = templateProperties.namespace.value.value;
+			if ( props.namespace ) {
+				const ns = props.namespace.value.value;
 				validator.namespace = namespaces[ ns ] || ns;
 			}
 
@@ -72,4 +72,12 @@ export default function validateJs ( validator, js ) {
 			});
 		}
 	});
+
+	[ 'methods', 'helpers' ].forEach( key => {
+		if ( validator.properties[ key ] ) {
+			validator.properties[ key ].value.properties.forEach( prop => {
+				validator[ key ][ prop.key.name ] = prop.value;
+			});
+		}
+	});
 }

src/validate/js/propValidators/helpers.js

@@ -1,5 +1,6 @@
 import checkForDupes from '../utils/checkForDupes.js';
 import checkForComputedKeys from '../utils/checkForComputedKeys.js';
+import { walk } from 'estree-walker';
 
 export default function helpers ( validator, prop ) {
 	if ( prop.value.type !== 'ObjectExpression' ) {
@@ -9,4 +10,45 @@ export default function helpers ( validator, prop ) {
 
 	checkForDupes( validator, prop.value.properties );
 	checkForComputedKeys( validator, prop.value.properties );
+
+	prop.value.properties.forEach( prop => {
+		if ( !/FunctionExpression/.test( prop.value.type ) ) return;
+
+		let lexicalDepth = 0;
+		let usesArguments = false;
+
+		walk( prop.value.body, {
+			enter ( node ) {
+				if ( /^Function/.test( node.type ) ) {
+					lexicalDepth += 1;
+				}
+
+				else if ( lexicalDepth === 0 ) {
+					// handle special case that's caused some people confusion — using `this.get(...)` instead of passing argument
+					// TODO do the same thing for computed values?
+					if ( node.type === 'CallExpression' && node.callee.type === 'MemberExpression' && node.callee.object.type === 'ThisExpression' && node.callee.property.name === 'get' && !node.callee.property.computed ) {
+						validator.error( `Cannot use this.get(...) — it must be passed into the helper function as an argument`, node.start );
+					}
+
+					if ( node.type === 'ThisExpression' ) {
+						validator.error( `Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?`, node.start );
+					}
+
+					else if ( node.type === 'Identifier' && node.name === 'arguments' ) {
+						usesArguments = true;
+					}
+				}
+			},
+
+			leave ( node ) {
+				if ( /^Function/.test( node.type ) ) {
+					lexicalDepth -= 1;
+				}
+			}
+		});
+
+		if ( prop.value.params.length === 0 && !usesArguments ) {
+			validator.warn( `Helpers should be pure functions, with at least one argument`, prop.start );
+		}
+	});
 }

src/validate/js/propValidators/namespace.js

@@ -1,5 +1,22 @@
+import * as namespaces from '../../../utils/namespaces.js';
+import FuzzySet from '../utils/FuzzySet.js';
+
+const fuzzySet = new FuzzySet( namespaces.validNamespaces );
+const valid = new Set( namespaces.validNamespaces );
+
 export default function namespace ( validator, prop ) {
-	if ( prop.value.type !== 'Literal' || typeof prop.value.value !== 'string' ) {
+	const ns = prop.value.value;
+
+	if ( prop.value.type !== 'Literal' || typeof ns !== 'string' ) {
 		validator.error( `The 'namespace' property must be a string literal representing a valid namespace`, prop.start );
 	}
+
+	if ( !valid.has( ns ) ) {
+		const matches = fuzzySet.get( ns );
+		if ( matches && matches[0] && matches[0][0] > 0.7 ) {
+			validator.error( `Invalid namespace '${ns}' (did you mean '${matches[0][1]}'?)`, prop.start );
+		} else {
+			validator.error( `Invalid namespace '${ns}'`, prop.start );
+		}
+	}
 }

test/validator/samples/helper-purity-check-needs-arguments/input.html

@@ -0,0 +1,11 @@
+{{foo()}}
+
+<script>
+	export default {
+		helpers: {
+			foo () {
+				return Math.random();
+			}
+		}
+	}
+</script>

test/validator/samples/helper-purity-check-needs-arguments/warnings.json

@@ -0,0 +1,8 @@
+[{
+	"message": "Helpers should be pure functions, with at least one argument",
+	"pos": 54,
+	"loc": {
+		"line": 6,
+		"column": 3
+	}
+}]

test/validator/samples/helper-purity-check-no-this/errors.json

@@ -0,0 +1,8 @@
+[{
+	"message": "Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?",
+	"pos": 95,
+	"loc": {
+		"line": 7,
+		"column": 4
+	}
+}]

test/validator/samples/helper-purity-check-no-this/input.html

@@ -0,0 +1,11 @@
+<button on:click='foo()'>foo</button>
+
+<script>
+	export default {
+		helpers: {
+			foo () {
+				this.set({ foo: true });
+			}
+		}
+	}
+</script>

test/validator/samples/helper-purity-check-this-get/errors.json

@@ -0,0 +1,8 @@
+[{
+	"message": "Cannot use this.get(...) — it must be passed into the helper function as an argument",
+	"pos": 74,
+	"loc": {
+		"line": 7,
+		"column": 11
+	}
+}]

test/validator/samples/helper-purity-check-this-get/input.html

@@ -0,0 +1,11 @@
+{{foo()}}
+
+<script>
+	export default {
+		helpers: {
+			foo () {
+				return this.get( 'bar' );
+			}
+		}
+	}
+</script>

test/validator/samples/helper-purity-check-uses-arguments/input.html

@@ -0,0 +1,13 @@
+{{sum(1, 2, 3)}}
+
+<script>
+	export default {
+		helpers: {
+			sum () {
+				var total = '';
+				for ( var i = 0; i < arguments.length; i += 1 ) total += arguments[i];
+				return total;
+			}
+		}
+	}
+</script>

test/validator/samples/helper-purity-check-uses-arguments/warnings.json

@@ -0,0 +1 @@
+[]

test/validator/samples/method-nonexistent-helper/errors.json

@@ -0,0 +1,8 @@
+[{
+	"message": "'foo' is an invalid callee (should be one of this.*, event.*, set, fire or bar). 'foo' exists on 'helpers', did you put it in the wrong place?",
+	"pos": 18,
+	"loc": {
+		"line": 1,
+		"column": 18
+	}
+}]

test/validator/samples/method-nonexistent-helper/input.html

@@ -0,0 +1,17 @@
+<button on:click='foo()'></button>
+
+<script>
+	export default {
+		methods: {
+			bar () {
+				// ...
+			}
+		},
+
+		helpers: {
+			foo ( x ) {
+				return x;
+			}
+		}
+	};
+</script>

test/validator/samples/method-nonexistent/errors.json

@@ -0,0 +1,8 @@
+[{
+	"message": "'foo' is an invalid callee (should be one of this.*, event.*, set, fire or bar)",
+	"pos": 18,
+	"loc": {
+		"line": 1,
+		"column": 18
+	}
+}]

test/validator/samples/method-nonexistent/input.html

@@ -0,0 +1,11 @@
+<button on:click='foo()'></button>
+
+<script>
+	export default {
+		methods: {
+			bar () {
+				// ...
+			}
+		}
+	};
+</script>