From 7245ccf8f9555c007e75438474d727ce6ffc7cd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 28 Jun 2019 15:48:33 +0200 Subject: [PATCH] [IMP] component: various prop validation improvements - add optional form for list props: ['optionalField?'] - accept undefined values for optional props - allow declaring props with only boolean true - throw error if extra prop is given to component closes #223 --- doc/component.md | 57 +++++----- doc/tooling.md | 3 + src/component.ts | 91 +-------------- src/qweb_extensions.ts | 103 ++++++++++++++++- tests/__snapshots__/component.test.ts.snap | 2 +- .../props_validation.test.ts.snap | 43 +++++++ tests/props_validation.test.ts | 105 +++++++++++++++--- 7 files changed, 277 insertions(+), 127 deletions(-) create mode 100644 tests/__snapshots__/props_validation.test.ts.snap diff --git a/doc/component.md b/doc/component.md index ab69d1e7..509105c0 100644 --- a/doc/component.md +++ b/doc/component.md @@ -119,24 +119,23 @@ find a template with the component name (or one of its ancestor). ```js class Counter extends owl.Component { - static props = { - initialValue: Number, - optional: true, - }; + static props = { + initialValue: Number, + optional: true + }; } ``` - -- **`defaultProps`** (Object, optional): if given, this object define default +* **`defaultProps`** (Object, optional): if given, this object define default values for (top-level) props. Whenever `props` are given to the object, they will be altered to add default value (if missing). Note that it does not change the initial object, a new object will be created instead. ```js class Counter extends owl.Component { - static defaultProps = { - initialValue: 0 - }; + static defaultProps = { + initialValue: 0 + }; } ``` @@ -179,7 +178,7 @@ We explain here all the public methods of the `Component` class. framework instead. Obviously, these methods are reserved for Owl, and should not be used by Owl -users, unless they want to override them. Also, Owl reserves all method names +users, unless they want to override them. Also, Owl reserves all method names starting with `__`, in order to prevent possible future conflicts with user code whenever Owl needs to change. @@ -372,22 +371,22 @@ easy to test a component. Updating the environment is not as simple as changing a component's state: its content is not observed, so updates will not be reflected immediately in the -user interface. There is however a mechanism to force root widgets to rerender +user interface. There is however a mechanism to force root widgets to rerender themselves whenever the environment is modified: one only needs to trigger the -`update` event on the QWeb instance. For example, a responsive environment +`update` event on the QWeb instance. For example, a responsive environment could be programmed like this: ```js function setupResponsivePlugin(env) { - const isMobile = () => window.innerWidth <= 768; - env.isMobile = isMobile(); - const updateEnv = owl.utils.debounce(() => { - if (env.isMobile !== isMobile()) { - env.isMobile = !env.isMobile; - env.qweb.trigger('update'); - } - }, 15); - window.addEventListener("resize", updateEnv); + const isMobile = () => window.innerWidth <= 768; + env.isMobile = isMobile(); + const updateEnv = owl.utils.debounce(() => { + if (env.isMobile !== isMobile()) { + env.isMobile = !env.isMobile; + env.qweb.trigger("update"); + } + }, 15); + window.addEventListener("resize", updateEnv); } ``` @@ -805,7 +804,8 @@ of the props. Here is how it works in Owl: - props are validated whenever a component is created/updated - props are only validated in `dev` mode (see [tooling page](tooling.md#development-mode)) - if a key does not match the description, an error is thrown -- it only validates keys defined in (static) `props`. Additional keys in (component) `props` are not validated. +- it validates keys defined in (static) `props`. Additional keys given by the + parent will cause an error. For example: @@ -833,15 +833,16 @@ class ComponentB extends owl.Component { - it is an object or a list of strings - a list of strings is a simplified props definition, which only lists the name - of the props. + of the props. Also, if the name ends with `?`, it is considered optional. - all props are by default required, unless they are defined with `optional: true` (in that case, validation is only done if there is a value) - valid types are: `Number, String, Boolean, Object, Array, Date, Function`, and all constructor functions (so, if you have a `Person` class, it can be used as a type) - arrays are homogeneous (all elements have the same type/shape) -For each key, a `prop` definition is either a constructor, a list of constructors, or an object: +For each key, a `prop` definition is either a boolean, a constructor, a list of constructors, or an object: +- a boolean: indicate that the props exists, and is mandatory. - a constructor: this should describe the type, for example: `id: Number` describe the props `id` as a number - a list of constructors. In that case, this means that we allow more than one @@ -859,6 +860,11 @@ Examples: static props = ['message', 'id', 'date']; ``` +```js + // size is optional + static props = ['message', 'size?']; +``` + ```js static props = { messageIds: {type: Array, element: Number}, // list of number @@ -873,7 +879,8 @@ Examples: url: String ]}, // object, with keys id (number), name (string, optional) and url (string) someFlag: Boolean, // a boolean, mandatory (even if `false`) - someVal: [Boolean, Date] // either a boolean or a date + someVal: [Boolean, Date], // either a boolean or a date + otherValue: true, // indicates that it is a prop }; ``` diff --git a/doc/tooling.md b/doc/tooling.md index 1baa954e..3c17e7b1 100644 --- a/doc/tooling.md +++ b/doc/tooling.md @@ -37,6 +37,9 @@ owl.__info__.mode = "dev"; Note that templates compiled with the `prod` settings will not be recompiled. So, changing this setting is best done at startup. +An important job done by the `dev` mode is to validate props for each component +creation and update. Also, extra props will cause an error. + ## Playground The playground is an important application designed to help learning and diff --git a/src/component.ts b/src/component.ts index 43d77e05..98147c68 100644 --- a/src/component.ts +++ b/src/component.ts @@ -1,5 +1,5 @@ import { Observer } from "./observer"; -import { QWeb, CompiledTemplate } from "./qweb_core"; +import { QWeb, CompiledTemplate, UTILS } from "./qweb_core"; import { h, patch, VNode } from "./vdom"; /** @@ -119,9 +119,6 @@ export class Component { if (defaultProps) { props = this.__applyDefaultProps(props, defaultProps); } - if (QWeb.dev) { - this.__validateProps(props || {}); - } // is this a good idea? // Pro: if props is empty, we can create easily a component // Con: this is not really safe @@ -135,6 +132,11 @@ export class Component { parent.__owl__.children[id] = this; } else { this.env = parent; + if (QWeb.dev) { + // we only validate props for root widgets here. "Regular" widget + // props are validated by the t-component directive + UTILS.validateProps(this.constructor, this.props); + } this.env.qweb.on("update", this, () => { if (this.__owl__.isMounted) { this.render(true); @@ -423,9 +425,6 @@ export class Component { if (defaultProps) { nextProps = this.__applyDefaultProps(nextProps, defaultProps); } - if (QWeb.dev) { - this.__validateProps(nextProps); - } await this.willUpdateProps(nextProps); this.props = nextProps; await this.render(forceUpdate, patchQueue); @@ -591,82 +590,4 @@ export class Component { patch[0].patched(patch[2]); } } - - /** - * Validate the component props (or next props) against the (static) props - * description. This is potentially an expensive operation: it may needs to - * visit recursively the props and all the children to check if they are valid. - * This is why it is only done in 'dev' mode. - */ - __validateProps(props: Object) { - const propsDef = (this.constructor).props; - if (propsDef instanceof Array) { - // list of strings (prop names) - for (let i = 0, l = propsDef.length; i < l; i++) { - if (!(propsDef[i] in props)) { - throw new Error(`Missing props '${propsDef[i]}' (component '${this.constructor.name}')`); - } - } - } else if (propsDef) { - // propsDef is an object now - for (let propName in propsDef) { - if (!(propName in props)) { - if (propsDef[propName] && !propsDef[propName].optional) { - throw new Error(`Missing props '${propName}' (component '${this.constructor.name}')`); - } else { - break; - } - } - let isValid = isValidProp(props[propName], propsDef[propName]); - if (!isValid) { - throw new Error( - `Props '${propName}' of invalid type in component '${this.constructor.name}'` - ); - } - } - } - } -} - -//------------------------------------------------------------------------------ -// Prop validation helper -//------------------------------------------------------------------------------ - -/** - * Check if an invidual prop value matches its (static) prop definition - */ -function isValidProp(prop, propDef): boolean { - if (typeof propDef === "function") { - // Check if a value is constructed by some Constructor. Note that there is a - // slight abuse of language: we want to consider primitive values as well. - // - // So, even though 1 is not an instance of Number, we want to consider that - // it is valid. - if (typeof prop === "object") { - return prop instanceof propDef; - } - return typeof prop === propDef.name.toLowerCase(); - } else if (propDef instanceof Array) { - // If this code is executed, this means that we want to check if a prop - // matches at least one of its descriptor. - let result = false; - for (let i = 0, iLen = propDef.length; i < iLen; i++) { - result = result || isValidProp(prop, propDef[i]); - } - return result; - } - // propsDef is an object - let result = isValidProp(prop, propDef.type); - if (propDef.type === Array) { - for (let i = 0, iLen = prop.length; i < iLen; i++) { - result = result && isValidProp(prop[i], propDef.element); - } - } - if (propDef.type === Object) { - const shape = propDef.shape; - for (let key in shape) { - result = result && isValidProp(prop[key], shape[key]); - } - } - return result; } diff --git a/src/qweb_extensions.ts b/src/qweb_extensions.ts index 0754913b..565be819 100644 --- a/src/qweb_extensions.ts +++ b/src/qweb_extensions.ts @@ -471,7 +471,7 @@ QWeb.addDirective({ if (tattClass) { let tattExpr = ctx.formatExpression(tattClass); if (tattExpr[0] !== "{" || tattExpr[tattExpr.length - 1] !== "}") { - tattExpr = `this.utils.toObj(${tattExpr})`; + tattExpr = `utils.toObj(${tattExpr})`; } if (classAttr) { ctx.addLine(`Object.assign(${classObj}, ${tattExpr})`); @@ -545,11 +545,13 @@ QWeb.addDirective({ ctx.addLine( `let W${componentID} = context.components && context.components[componentKey${componentID}] || QWeb.components[componentKey${componentID}];` ); - // maybe only do this in dev mode... ctx.addLine( `if (!W${componentID}) {throw new Error('Cannot find the definition of component "' + componentKey${componentID} + '"')}` ); + if (QWeb.dev) { + ctx.addLine(`utils.validateProps(W${componentID}, props${componentID})`); + } ctx.addLine(`w${componentID} = new W${componentID}(owner, props${componentID});`); ctx.addLine(`context.__owl__.cmap[${templateID}] = w${componentID}.__owl__.id;`); @@ -590,6 +592,9 @@ QWeb.addDirective({ ctx.addElse(); // need to update component const patchQueueCode = async ? `patchQueue${componentID}` : "extra.patchQueue"; + if (QWeb.dev) { + ctx.addLine(`utils.validateProps(w${componentID}.constructor, props${componentID})`); + } ctx.addLine( `def${defID} = def${defID} || w${componentID}.__updateProps(props${componentID}, extra.forceUpdate, ${patchQueueCode});` ); @@ -626,6 +631,100 @@ QWeb.addDirective({ } }); +//------------------------------------------------------------------------------ +// Prop validation helper +//------------------------------------------------------------------------------ + +/** + * Validate the component props (or next props) against the (static) props + * description. This is potentially an expensive operation: it may needs to + * visit recursively the props and all the children to check if they are valid. + * This is why it is only done in 'dev' mode. + */ +UTILS.validateProps = function(Widget, props: Object) { + const propsDef = (Widget).props; + if (propsDef instanceof Array) { + // list of strings (prop names) + for (let i = 0, l = propsDef.length; i < l; i++) { + const propName = propsDef[i]; + if (propName[propName.length - 1] === "?") { + // optional prop + break; + } + if (!props[propName]) { + throw new Error(`Missing props '${propsDef[i]}' (component '${Widget.name}')`); + } + } + for (let key in props) { + if (!propsDef.includes(key) && !propsDef.includes(key + "?")) { + throw new Error(`Unknown prop '${key}' given to component '${Widget.name}'`); + } + } + } else if (propsDef) { + // propsDef is an object now + for (let propName in propsDef) { + if (props[propName] === undefined) { + if (propsDef[propName] && !propsDef[propName].optional) { + throw new Error(`Missing props '${propName}' (component '${Widget.name}')`); + } else { + break; + } + } + let isValid = isValidProp(props[propName], propsDef[propName]); + if (!isValid) { + throw new Error(`Props '${propName}' of invalid type in component '${Widget.name}'`); + } + } + for (let propName in props) { + if (!(propName in propsDef)) { + throw new Error(`Unknown prop '${propName}' given to component '${Widget.name}'`); + } + } + } +}; + +/** + * Check if an invidual prop value matches its (static) prop definition + */ +function isValidProp(prop, propDef): boolean { + if (propDef === true) { + return true; + } + if (typeof propDef === "function") { + // Check if a value is constructed by some Constructor. Note that there is a + // slight abuse of language: we want to consider primitive values as well. + // + // So, even though 1 is not an instance of Number, we want to consider that + // it is valid. + if (typeof prop === "object") { + return prop instanceof propDef; + } + return typeof prop === propDef.name.toLowerCase(); + } else if (propDef instanceof Array) { + // If this code is executed, this means that we want to check if a prop + // matches at least one of its descriptor. + let result = false; + for (let i = 0, iLen = propDef.length; i < iLen; i++) { + result = result || isValidProp(prop, propDef[i]); + } + return result; + } + // propsDef is an object + let result = isValidProp(prop, propDef.type); + if (propDef.type === Array) { + for (let i = 0, iLen = prop.length; i < iLen; i++) { + result = result && isValidProp(prop[i], propDef.element); + } + } + if (propDef.type === Object) { + const shape = propDef.shape; + for (let key in shape) { + result = result && isValidProp(prop[key], shape[key]); + } + } + return result; +} + //------------------------------------------------------------------------------ // t-mounted //------------------------------------------------------------------------------ diff --git a/tests/__snapshots__/component.test.ts.snap b/tests/__snapshots__/component.test.ts.snap index fc212c2a..aa8a5cd3 100644 --- a/tests/__snapshots__/component.test.ts.snap +++ b/tests/__snapshots__/component.test.ts.snap @@ -353,7 +353,7 @@ exports[`class and style attributes with t-component t-att-class is properly add let def3; const ref5 = \`child\`; let _6 = {'a':true}; - Object.assign(_6, this.utils.toObj(context['state'].b?'b':'')) + Object.assign(_6, utils.toObj(context['state'].b?'b':'')) let w4 = 4 in context.__owl__.cmap ? context.__owl__.children[context.__owl__.cmap[4]] : false; let _2_index = c1.length; c1.push(null); diff --git a/tests/__snapshots__/props_validation.test.ts.snap b/tests/__snapshots__/props_validation.test.ts.snap new file mode 100644 index 00000000..49c7e0f7 --- /dev/null +++ b/tests/__snapshots__/props_validation.test.ts.snap @@ -0,0 +1,43 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`props validation props are validated in dev mode (code snapshot) 1`] = ` +"function anonymous(context,extra +) { + let utils = this.utils; + let QWeb = this.constructor; + let owner = context; + var h = this.utils.h; + let c1 = [], p1 = {key:1}; + var vn1 = h('div', p1, c1); + //COMPONENT + let def3; + let w4 = 4 in context.__owl__.cmap ? context.__owl__.children[context.__owl__.cmap[4]] : false; + let _2_index = c1.length; + c1.push(null); + let props4 = {message:1}; + if (w4 && w4.__owl__.renderPromise && !w4.__owl__.vnode) { + if (utils.shallowEqual(props4, w4.__owl__.renderProps)) { + def3 = w4.__owl__.renderPromise; + } else { + w4.destroy(); + w4 = false; + } + } + if (!w4) { + let componentKey4 = \`Child\`; + let W4 = context.components && context.components[componentKey4] || QWeb.components[componentKey4]; + if (!W4) {throw new Error('Cannot find the definition of component \\"' + componentKey4 + '\\"')} + utils.validateProps(W4, props4) + w4 = new W4(owner, props4); + context.__owl__.cmap[4] = w4.__owl__.id; + def3 = w4.__prepare(); + def3 = def3.then(vnode=>{let pvnode=h(vnode.sel, {key: 4, hook: {insert(vn) {let nvn=w4.__mount(vnode, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w4.destroy();}}});c1[_2_index]=pvnode;w4.__owl__.pvnode = pvnode;}); + } else { + utils.validateProps(w4.constructor, props4) + def3 = def3 || w4.__updateProps(props4, extra.forceUpdate, extra.patchQueue); + def3 = def3.then(()=>{if (w4.__owl__.isDestroyed) {return};let pvnode=w4.__owl__.pvnode;c1[_2_index]=pvnode;}); + } + extra.promises.push(def3); + return vn1; +}" +`; diff --git a/tests/props_validation.test.ts b/tests/props_validation.test.ts index e7eb7952..8e7c1e47 100644 --- a/tests/props_validation.test.ts +++ b/tests/props_validation.test.ts @@ -1,6 +1,6 @@ import { Component, Env } from "../src/component"; import { makeTestFixture, makeTestEnv } from "./helpers"; -import { QWeb } from "../src"; +import { QWeb, UTILS } from "../src/qweb_core"; //------------------------------------------------------------------------------ // Setup and helpers @@ -45,19 +45,6 @@ describe("props validation", () => { }).not.toThrow(); }); - test("props validation is also done on update props", async () => { - expect.assertions(1); - class TestWidget extends Widget { - static props = ["message"]; - } - const w = new TestWidget(env, { message: "bottle" }); - try { - await w.__updateProps({}); - } catch (e) { - expect(e.message).toBe("Missing props 'message' (component 'TestWidget')"); - } - }); - test("props: list of strings", async () => { class TestWidget extends Widget { static props = ["message"]; @@ -234,6 +221,96 @@ describe("props validation", () => { new TestWidget(env, { p: { id: 1, url: [12, true] } }); }).toThrow(); }); + + test("props are validated in dev mode (code snapshot)", async () => { + env.qweb.addTemplates(` + +
+ +
+
+ `); + + class Child extends Widget { + static props = ["message"]; + } + class App extends Widget { + components = { Child }; + } + const app = new App(env); + await app.mount(fixture); + expect(fixture.innerHTML).toBe("
1
"); + // need to make sure there are 2 call to update props. one at component + // creation, and one at update time. + expect(env.qweb.templates.App.fn.toString()).toMatchSnapshot(); + }); + + test("props: list of strings with optional props", async () => { + class TestWidget extends Widget { + static props = ["message", "someProp?"]; + } + + expect(() => { + UTILS.validateProps(TestWidget, { someProp: 1 }); + }).toThrow(); + expect(() => { + UTILS.validateProps(TestWidget, { message: 1 }); + }).not.toThrow(); + }); + + test("props: can be defined with a boolean", async () => { + class TestWidget extends Widget { + static props = { message: true }; + } + + expect(() => { + UTILS.validateProps(TestWidget, {}); + }).toThrow(); + }); + + test("props: extra props cause an error", async () => { + class TestWidget extends Widget { + static props = ["message"]; + } + + expect(() => { + UTILS.validateProps(TestWidget, { message: 1, flag: true }); + }).toThrow(); + }); + + test("props: extra props cause an error, part 2", async () => { + class TestWidget extends Widget { + static props = {message: true}; + } + + expect(() => { + UTILS.validateProps(TestWidget, { message: 1, flag: true }); + }).toThrow(); + }); + + test("props: optional prop do not cause an error", async () => { + class TestWidget extends Widget { + static props = ["message?"]; + } + + expect(() => { + UTILS.validateProps(TestWidget, { message: 1}); + }).not.toThrow(); + }); + + test("optional prop do not cause an error if value is undefined", async () => { + class TestWidget extends Widget { + static props = {message: {type: String, optional: true}}; + } + + expect(() => { + UTILS.validateProps(TestWidget, { message: undefined}); + }).not.toThrow(); + expect(() => { + UTILS.validateProps(TestWidget, { message: null}); + }).toThrow(); + }); + }); describe("default props", () => {