[IMP] props validation: cannot set default value on mandatory props

CHANGELOG.md

@@ -30,6 +30,8 @@ All changes are documented here in no particular order.
 - breaking: `catchError` method is replaced by `onError` hook ([details](#36-catcherror-method-is-replaced-by-onerror-hook))
 - breaking: Support for inline css (`css` tag and static `style`) has been removed ([details](#37-support-for-inline-css-css-tag-and-static-style-has-been-removed))
 - new: prop validation system can now describe that additional props are allowed (with `*`) ([doc](doc/reference/props.md#props-validation))
+- breaking: prop validation system does not allow default prop on a mandatory (not optional) prop ([doc](doc/reference/props.md#props-validation))
+
 
 
 **Templates**

doc/reference/props.md

@@ -168,11 +168,15 @@ For each key, a `prop` definition is either a boolean, a constructor, a list of
   - `shape`: if the type was `Object`, then the `shape` key describes the interface of the object. If it is not set, then we only validate the object, not its elements,
   - `validate`: this is a function which should return a boolean to determine if
     the value is valid or not. Useful for custom validation logic.
+  - `optional`: if true, the prop is not mandatory
 
 There is a special `*` prop that means that additional prop are allowed. This is
 sometimes useful for generic components that will propagate some or all their
 props to their child components.
 
+Note that default values cannot be defined for a mandatory props. Doing so will
+result in a prop validation error.
+
 Examples:
 
 ```js
@@ -190,7 +194,8 @@ class ComponentB extends owl.Component {
       element: {type: Object, shape: {id: Boolean, text: String }
     },
    date: Date,
-   combinedVal: [Number, Boolean]
+   combinedVal: [Number, Boolean],
+   optionalProp: { type: Number, optional: true }
   };
 
   ...

src/component/props_validation.ts

@@ -47,6 +47,7 @@ export function validateProps<P>(name: string | ComponentConstructor<P>, props:
   }
   applyDefaultProps(props, ComponentClass);
 
+  const defaultProps = ComponentClass.defaultProps || {};
   let propsDef = getPropDescription(ComponentClass.props);
   const allowAdditionalProps = "*" in propsDef;
 
@@ -54,8 +55,18 @@ export function validateProps<P>(name: string | ComponentConstructor<P>, props:
     if (propName === "*") {
       continue;
     }
+    const propDef = propsDef[propName];
+    let isMandatory = !!propDef;
+    if (typeof propDef === "object" && "optional" in propDef) {
+      isMandatory = !propDef.optional;
+    }
+    if (isMandatory && propName in defaultProps) {
+      throw new Error(
+        `A default value cannot be defined for a mandatory prop (name: '${propName}', component: ${ComponentClass.name}`
+      );
+    }
     if ((props as any)[propName] === undefined) {
-      if (propsDef[propName] && !propsDef[propName].optional) {
+      if (isMandatory) {
         throw new Error(`Missing props '${propName}' (component '${ComponentClass.name}')`);
       } else {
         continue;
@@ -63,7 +74,7 @@ export function validateProps<P>(name: string | ComponentConstructor<P>, props:
     }
     let isValid;
     try {
-      isValid = isValidProp((props as any)[propName], propsDef[propName]);
+      isValid = isValidProp((props as any)[propName], propDef);
     } catch (e) {
       (e as Error).message = `Invalid prop '${propName}' in component ${ComponentClass.name} (${
         (e as Error).message

tests/components/__snapshots__/props_validation.test.ts.snap

@@ -1,6 +1,19 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`default props can set default required boolean values 1`] = `
+exports[`default props a default prop cannot be defined on a mandatory prop 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const props1 = {};
+    helpers.validateProps(\`Child\`, props1, ctx);
+    return component(\`Child\`, props1, key + \`__1\`, node, ctx);
+  }
+}"
+`;
+
+exports[`default props can set default boolean values 1`] = `
 "function anonymous(bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
@@ -16,7 +29,7 @@ exports[`default props can set default required boolean values 1`] = `
 }"
 `;
 
-exports[`default props can set default required boolean values 2`] = `
+exports[`default props can set default boolean values 2`] = `
 "function anonymous(bdom, helpers
 ) {
   let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;

tests/components/props_validation.test.ts

@@ -684,7 +684,7 @@ describe("props validation", () => {
   test("default values are applied before validating props at update", async () => {
     // need to do something about errors catched in render
     class SubComp extends Component {
-      static props = { p: { type: Number } };
+      static props = { p: { type: Number, optional: true } };
       static template = xml`<div><t t-esc="props.p"/></div>`;
       static defaultProps = { p: 4 };
     }
@@ -791,9 +791,9 @@ describe("default props", () => {
     expect(fixture.innerHTML).toBe("<div><div>4</div></div>");
   });
 
-  test("can set default required boolean values", async () => {
+  test("can set default boolean values", async () => {
     class SubComp extends Component {
-      static props = ["p", "q"];
+      static props = ["p?", "q?"];
       static defaultProps = { p: true, q: false };
       static template = xml`<span><t t-if="props.p">hey</t><t t-if="!props.q">hey</t></span>`;
     }
@@ -804,4 +804,28 @@ describe("default props", () => {
     await mount(Parent, fixture, { dev: true });
     expect(fixture.innerHTML).toBe("<div><span>heyhey</span></div>");
   });
+
+  test("a default prop cannot be defined on a mandatory prop", async () => {
+    class Child extends Component {
+      static props = {
+        mandatory: Number,
+      };
+      static defaultProps = { mandatory: 3 };
+      static template = xml` <div><t t-esc="props.mandatory"/></div>`;
+    }
+    class Parent extends Component {
+      static components = { Child };
+      static template = xml`<Child/>`;
+    }
+    let error: Error;
+    try {
+      await mount(Parent, fixture, { dev: true });
+    } catch (e) {
+      error = e as Error;
+    }
+    expect(error!).toBeDefined();
+    expect(error!.message).toBe(
+      "A default value cannot be defined for a mandatory prop (name: 'mandatory', component: Child"
+    );
+  });
 });