[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

@@ -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
   };
 ```
 

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

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<T extends Env, Props extends {}, State extends {}> {
     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<T extends Env, Props extends {}, State extends {}> {
       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<T extends Env, Props extends {}, State extends {}> {
       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<T extends Env, Props extends {}, State extends {}> {
       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 = (<any>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;
 }

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 = (<any>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
 //------------------------------------------------------------------------------

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);

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;
+}"
+`;

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(`
+      <templates>
+        <div t-name="App">
+          <Child message="1"/>
+        </div>
+        <div t-name="Child"><t t-esc="props.message"/></div>
+      </templates>`);
+
+    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("<div><div>1</div></div>");
+    // 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", () => {