From 3595470305cecbd9b5e1418b2b3684e0d528eea4 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 12 Apr 2017 10:05:06 -0400 Subject: [PATCH 1/4] validate namespaces --- src/utils/namespaces.js | 5 +++++ src/validate/html/index.js | 2 +- src/validate/index.js | 4 +++- src/validate/js/index.js | 16 ++++++++-------- src/validate/js/propValidators/namespace.js | 19 ++++++++++++++++++- .../samples/namespace-invalid/errors.json | 8 ++++++++ .../samples/namespace-invalid/input.html | 5 +++++ 7 files changed, 48 insertions(+), 11 deletions(-) create mode 100644 test/validator/samples/namespace-invalid/errors.json create mode 100644 test/validator/samples/namespace-invalid/input.html diff --git a/src/utils/namespaces.js b/src/utils/namespaces.js index 2ab0b116f058..595d0f0defeb 100644 --- a/src/utils/namespaces.js +++ b/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 }; diff --git a/src/validate/html/index.js b/src/validate/html/index.js index 2e831ee9c101..f54c4d4fe5ce 100644 --- a/src/validate/html/index.js +++ b/src/validate/html/index.js @@ -18,7 +18,7 @@ export default function validateHtml ( validator, html ) { node.children.forEach( visit ); } - if (node.else ) { + if ( node.else ) { visit( node.else ); } diff --git a/src/validate/index.js b/src/validate/index.js index e1e536f4bd0e..4efdee8d47f2 100644 --- a/src/validate/index.js +++ b/src/validate/index.js @@ -36,7 +36,9 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file }); }, - namespace: null + namespace: null, + defaultExport: null, + properties: {} }; if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) { diff --git a/src/validate/js/index.js b/src/validate/js/index.js index cdef266c969b..d0fc396320bb 100644 --- a/src/validate/js/index.js +++ b/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; } diff --git a/src/validate/js/propValidators/namespace.js b/src/validate/js/propValidators/namespace.js index ca664261ef60..ec8bad0a0460 100644 --- a/src/validate/js/propValidators/namespace.js +++ b/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 ); + } + } } diff --git a/test/validator/samples/namespace-invalid/errors.json b/test/validator/samples/namespace-invalid/errors.json new file mode 100644 index 000000000000..b7f6d4d8981c --- /dev/null +++ b/test/validator/samples/namespace-invalid/errors.json @@ -0,0 +1,8 @@ +[{ + "message": "Invalid namespace 'http://www.w3.org/1999/svg' (did you mean 'http://www.w3.org/2000/svg'?)", + "pos": 29, + "loc": { + "line": 3, + "column": 2 + } +}] diff --git a/test/validator/samples/namespace-invalid/input.html b/test/validator/samples/namespace-invalid/input.html new file mode 100644 index 000000000000..a2e5baf39b01 --- /dev/null +++ b/test/validator/samples/namespace-invalid/input.html @@ -0,0 +1,5 @@ + \ No newline at end of file From c7ac8b82ba51e6d3df8515545bd59534e01b8722 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 12 Apr 2017 10:42:55 -0400 Subject: [PATCH 2/4] check that event handler callee corresponds to a valid method --- src/validate/html/index.js | 30 +++++++++++++++++++ src/validate/index.js | 6 +++- src/validate/js/index.js | 8 +++++ .../method-nonexistent-helper/errors.json | 8 +++++ .../method-nonexistent-helper/input.html | 17 +++++++++++ .../samples/method-nonexistent/errors.json | 8 +++++ .../samples/method-nonexistent/input.html | 11 +++++++ 7 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 test/validator/samples/method-nonexistent-helper/errors.json create mode 100644 test/validator/samples/method-nonexistent-helper/input.html create mode 100644 test/validator/samples/method-nonexistent/errors.json create mode 100644 test/validator/samples/method-nonexistent/input.html diff --git a/src/validate/html/index.js b/src/validate/html/index.js index f54c4d4fe5ce..8cad9bd849a7 100644 --- a/src/validate/html/index.js +++ b/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,6 +18,30 @@ 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 ) { diff --git a/src/validate/index.js b/src/validate/index.js index 4efdee8d47f2..f0a274c9eb89 100644 --- a/src/validate/index.js +++ b/src/validate/index.js @@ -36,9 +36,13 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file }); }, + source, + namespace: null, defaultExport: null, - properties: {} + properties: {}, + methods: {}, + helpers: {} }; if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) { diff --git a/src/validate/js/index.js b/src/validate/js/index.js index d0fc396320bb..3f7e8c0e427c 100644 --- a/src/validate/js/index.js +++ b/src/validate/js/index.js @@ -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; + }); + } + }); } diff --git a/test/validator/samples/method-nonexistent-helper/errors.json b/test/validator/samples/method-nonexistent-helper/errors.json new file mode 100644 index 000000000000..80183498ca9e --- /dev/null +++ b/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 + } +}] diff --git a/test/validator/samples/method-nonexistent-helper/input.html b/test/validator/samples/method-nonexistent-helper/input.html new file mode 100644 index 000000000000..abb756f6c3ab --- /dev/null +++ b/test/validator/samples/method-nonexistent-helper/input.html @@ -0,0 +1,17 @@ + + + diff --git a/test/validator/samples/method-nonexistent/errors.json b/test/validator/samples/method-nonexistent/errors.json new file mode 100644 index 000000000000..2eaf6c215d83 --- /dev/null +++ b/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 + } +}] diff --git a/test/validator/samples/method-nonexistent/input.html b/test/validator/samples/method-nonexistent/input.html new file mode 100644 index 000000000000..75f2028b92f7 --- /dev/null +++ b/test/validator/samples/method-nonexistent/input.html @@ -0,0 +1,11 @@ + + + From fa65f7af6085feb223826e0a2f1edd8c95a8ad45 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 12 Apr 2017 11:23:08 -0400 Subject: [PATCH 3/4] enforce helper function purity --- src/validate/index.js | 44 +++++++++++-------- src/validate/js/propValidators/helpers.js | 36 +++++++++++++++ .../input.html | 11 +++++ .../warnings.json | 8 ++++ .../helper-purity-check-no-this/errors.json | 8 ++++ .../helper-purity-check-no-this/input.html | 11 +++++ .../input.html | 13 ++++++ .../warnings.json | 1 + 8 files changed, 114 insertions(+), 18 deletions(-) create mode 100644 test/validator/samples/helper-purity-check-needs-arguments/input.html create mode 100644 test/validator/samples/helper-purity-check-needs-arguments/warnings.json create mode 100644 test/validator/samples/helper-purity-check-no-this/errors.json create mode 100644 test/validator/samples/helper-purity-check-no-this/input.html create mode 100644 test/validator/samples/helper-purity-check-uses-arguments/input.html create mode 100644 test/validator/samples/helper-purity-check-uses-arguments/warnings.json diff --git a/src/validate/index.js b/src/validate/index.js index f0a274c9eb89..d83161168e38 100644 --- a/src/validate/index.js +++ b/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 ) => { @@ -45,25 +45,33 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file 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; + } } } diff --git a/src/validate/js/propValidators/helpers.js b/src/validate/js/propValidators/helpers.js index 6553f079729f..8248445d4493 100644 --- a/src/validate/js/propValidators/helpers.js +++ b/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,39 @@ 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 ) { + 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 ); + } + }); } diff --git a/test/validator/samples/helper-purity-check-needs-arguments/input.html b/test/validator/samples/helper-purity-check-needs-arguments/input.html new file mode 100644 index 000000000000..244c67e697b4 --- /dev/null +++ b/test/validator/samples/helper-purity-check-needs-arguments/input.html @@ -0,0 +1,11 @@ +{{foo()}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-needs-arguments/warnings.json b/test/validator/samples/helper-purity-check-needs-arguments/warnings.json new file mode 100644 index 000000000000..85abc4df9223 --- /dev/null +++ b/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 + } +}] \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-no-this/errors.json b/test/validator/samples/helper-purity-check-no-this/errors.json new file mode 100644 index 000000000000..9de3d7465d41 --- /dev/null +++ b/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": 74, + "loc": { + "line": 7, + "column": 11 + } +}] \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-no-this/input.html b/test/validator/samples/helper-purity-check-no-this/input.html new file mode 100644 index 000000000000..e950152de876 --- /dev/null +++ b/test/validator/samples/helper-purity-check-no-this/input.html @@ -0,0 +1,11 @@ +{{foo()}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-uses-arguments/input.html b/test/validator/samples/helper-purity-check-uses-arguments/input.html new file mode 100644 index 000000000000..559f0ba0c088 --- /dev/null +++ b/test/validator/samples/helper-purity-check-uses-arguments/input.html @@ -0,0 +1,13 @@ +{{sum(1, 2, 3)}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-uses-arguments/warnings.json b/test/validator/samples/helper-purity-check-uses-arguments/warnings.json new file mode 100644 index 000000000000..0637a088a01e --- /dev/null +++ b/test/validator/samples/helper-purity-check-uses-arguments/warnings.json @@ -0,0 +1 @@ +[] \ No newline at end of file From cc722f8f7a199ad72b17ff980a91fc69db0f7e97 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 12 Apr 2017 11:30:33 -0400 Subject: [PATCH 4/4] handle this.get special case in helpers --- src/validate/js/propValidators/helpers.js | 6 ++++++ .../samples/helper-purity-check-no-this/errors.json | 4 ++-- .../samples/helper-purity-check-no-this/input.html | 4 ++-- .../samples/helper-purity-check-this-get/errors.json | 8 ++++++++ .../samples/helper-purity-check-this-get/input.html | 11 +++++++++++ 5 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 test/validator/samples/helper-purity-check-this-get/errors.json create mode 100644 test/validator/samples/helper-purity-check-this-get/input.html diff --git a/src/validate/js/propValidators/helpers.js b/src/validate/js/propValidators/helpers.js index 8248445d4493..7e27e311344d 100644 --- a/src/validate/js/propValidators/helpers.js +++ b/src/validate/js/propValidators/helpers.js @@ -24,6 +24,12 @@ export default function helpers ( validator, prop ) { } 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 ); } diff --git a/test/validator/samples/helper-purity-check-no-this/errors.json b/test/validator/samples/helper-purity-check-no-this/errors.json index 9de3d7465d41..87c71a4034b8 100644 --- a/test/validator/samples/helper-purity-check-no-this/errors.json +++ b/test/validator/samples/helper-purity-check-no-this/errors.json @@ -1,8 +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": 74, + "pos": 95, "loc": { "line": 7, - "column": 11 + "column": 4 } }] \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-no-this/input.html b/test/validator/samples/helper-purity-check-no-this/input.html index e950152de876..795821b446de 100644 --- a/test/validator/samples/helper-purity-check-no-this/input.html +++ b/test/validator/samples/helper-purity-check-no-this/input.html @@ -1,10 +1,10 @@ -{{foo()}} + \ No newline at end of file