From 681983142d685f0d540953571a4975aa1ab77e89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 28 May 2019 09:54:25 +0200 Subject: [PATCH] [IMP] component: add props validation and default props system closes #116 --- doc/component.md | 102 ++++++++++++- src/component.ts | 128 +++++++++++++++- src/qweb_extensions.ts | 2 +- tests/props_validation.test.ts | 263 +++++++++++++++++++++++++++++++++ 4 files changed, 488 insertions(+), 7 deletions(-) create mode 100644 tests/props_validation.test.ts diff --git a/doc/component.md b/doc/component.md index 386213b9..61159cdf 100644 --- a/doc/component.md +++ b/doc/component.md @@ -7,9 +7,11 @@ - [Composition](#composition) - [Reference](#reference) - [Properties](#properties) + - [Static Properties](#static-properties) - [Methods](#methods) - [Lifecycle](#lifecycle) - [Semantics](#semantics) + - [Props Validation](#props-validation) - [Asynchronous rendering](#asynchronous-rendering) ## Overview @@ -144,11 +146,22 @@ find a template with the component name (or one of its ancestor). - **`props`** (Object): this is an object given (in the constructor) by the parent to configure the component. It can be dynamically changed later by the parent, in some case. Note that `props` are owned by the parent, not by the component. - As such, it should not ever be modified by the component. + As such, it should not ever be modified by the component!! - **`refs`** (Object): the `refs` object contains all references to sub DOM nodes or sub widgets defined by a `t-ref` directive in the component's template. +### Static Properties + +- **`props`** (Object, optional): if given, this is an object that describes the + type and shape of the (actual) props given to the component. If Owl mode is + `dev`, this will be used to validate the props each time the component is + created/updated. See [Props Validation](#props-validation) for more information. +- **`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. + ### Methods - **`mount(target)`** (async): this is the main way a component's hierarchy is added to the @@ -432,6 +445,93 @@ Here is what Owl will do: 6. `patched` hooks are called on `D`, `C` +### Props Validation + +As an application becomes complex, it may be quite unsafe to define props in an informal way. This leads to two issues: + +- hard to tell how a component should be used, by looking at its code. +- unsafe, it is easy to send wrong props into a component, either by refactoring a component, or one of its parent. + +A props type system would solve both issues, by describing the types and shapes +of the props. Here is how it works in Owl: + +- `props` key is a static key (so, different from `this.props` in a component instance) +- it is optional: it is ok for a component to not define a `props` key. +- 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. + +For example: + +```js +class ComponentA extends owl.Component { + static props = ['id', 'url']; + + ... +} + +class ComponentB extends owl.Component { + static props = { + count: {type: Number}, + messages: { + type: Array, + element: {type: Object, shape: {id: Boolean, text: 'string' } + }, + date: Date, + combinedVal: [Number, Boolean] + }; + + ... +} +``` + +- 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. +- 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: + +- 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 + type. For example, `id: [Number, String]` means that `id` can be either a string + or a number. +- an object. This makes it possible to have more expressive definition. The following sub keys are then allowed: + - `type`: the main type of the prop being validated + - `element`: if the type was `Array`, then the `element` key describes the type of each element in the array. It is optional (not set means that we only validate the array, not its elements), + - `shape`: if the type was `Object`, then the `shape` key describes the interface of the object. It is optional (not set means that we only validate the object, not its elements) + +Examples: + +```js + // only the existence of those 3 keys is documented + static props = ['message', 'id', 'date']; +``` + +```js + static props = { + messageIds: {type: Array, element: Number}, // list of number + otherArr: {type: Array}, // just array. no validation is made on sub elements + otherArr2: Array, // same as otherArr + someObj: {type: Object}, // just an object, no internal validation + someObj2: { + type: Object, + shape: { + id: Number, + name: {type: String, optional: true}, + 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 + }; +``` + ## Asynchronous rendering Working with asynchronous code always adds a lot of complexity to a system. Whenever diff --git a/src/component.ts b/src/component.ts index 9fc69f35..eb8f0f89 100644 --- a/src/component.ts +++ b/src/component.ts @@ -68,6 +68,11 @@ export class Component< env: T; state?: State; props: Props; + + // type of props is not easily representable in typescript... + static props?: any; + static defaultProps?: any; + refs: { [key: string]: Component | HTMLElement | undefined; } = {}; @@ -98,6 +103,13 @@ export class Component< constructor(parent: Component | T, props?: Props) { super(); + const defaultProps = (this.constructor).defaultProps; + 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 widget // Con: this is not really safe @@ -359,6 +371,13 @@ export class Component< ): Promise { const shouldUpdate = forceUpdate || this.shouldUpdate(nextProps); if (shouldUpdate) { + const defaultProps = (this.constructor).defaultProps; + 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); @@ -454,16 +473,16 @@ export class Component< */ _mount(vnode: VNode, elm: HTMLElement): VNode { this.__owl__.vnode = patch(elm, vnode); - if ( - this.__owl__.parent!.__owl__.isMounted && - !this.__owl__.isMounted - ) { + if (this.__owl__.parent!.__owl__.isMounted && !this.__owl__.isMounted) { this._callMounted(); } return this.__owl__.vnode; } - __mount() { + /** + * Only called by qweb t-widget directive (when t-keepalive is set) + */ + _remount() { if (!this.__owl__.isMounted) { this.__owl__.isMounted = true; this.mounted(); @@ -477,4 +496,103 @@ export class Component< this.__owl__.observer.notifyCB = this.render.bind(this); } } + + /** + * Apply default props (only top level). + * + * Note that this method does not modify in place the props, it returns a new + * prop object + */ + _applyDefaultProps(props: Object | undefined, defaultProps: Object): Props { + props = props ? Object.create(props) : {}; + for (let propName in defaultProps) { + if (props![propName] === undefined) { + props![propName] = defaultProps[propName]; + } + } + return props; + } + + /** + * 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]}' (widget '${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}' (widget '${this.constructor.name}')` + ); + } else { + break; + } + } + let isValid = isValidProp(props[propName], propsDef[propName]); + if (!isValid) { + throw new Error( + `Props '${propName}' of invalid type in widget '${ + 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; i < propDef.length; 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; i < prop.length; i++) { + result = result && isValidProp(prop[i], propDef.element); + } + } + if (propDef.type === Object) { + for (let key in propDef.shape) { + result = result && isValidProp(prop[key], propDef.shape[key]); + } + } + return result; } diff --git a/src/qweb_extensions.ts b/src/qweb_extensions.ts index a64f7428..6cd817e2 100644 --- a/src/qweb_extensions.ts +++ b/src/qweb_extensions.ts @@ -490,7 +490,7 @@ QWeb.addDirective({ ); let keepAliveCode = ""; if (keepAlive) { - keepAliveCode = `pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w${widgetID}.el,vn.elm);vn.elm=w${widgetID}.el;w${widgetID}.__mount();};`; + keepAliveCode = `pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w${widgetID}.el,vn.elm);vn.elm=w${widgetID}.el;w${widgetID}._remount();};`; } ctx.addLine( `def${defID} = def${defID}.then(()=>{if (w${widgetID}.__owl__.isDestroyed) {return};${ diff --git a/tests/props_validation.test.ts b/tests/props_validation.test.ts new file mode 100644 index 00000000..642b2406 --- /dev/null +++ b/tests/props_validation.test.ts @@ -0,0 +1,263 @@ +import { Component, Env } from "../src/component"; +import { makeTestFixture, makeTestEnv } from "./helpers"; +import { QWeb } from "../src"; + +//------------------------------------------------------------------------------ +// Setup and helpers +//------------------------------------------------------------------------------ + +let fixture: HTMLElement; +let env: Env; +let dev: boolean = false; + +beforeEach(() => { + fixture = makeTestFixture(); + env = makeTestEnv(); + dev = QWeb.dev; + QWeb.dev = true; +}); + +afterEach(() => { + fixture.remove(); + QWeb.dev = dev; +}); + +class Widget extends Component {} + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ +describe("props validation", () => { + test("validation is only done in dev mode", async () => { + class TestWidget extends Widget { + static props = ["message"]; + } + + QWeb.dev = true; + expect(() => { + new TestWidget(env); + }).toThrow(); + + QWeb.dev = false; + + expect(() => { + new TestWidget(env); + }).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' (widget 'TestWidget')"); + } + }); + + test("props: list of strings", async () => { + class TestWidget extends Widget { + static props = ["message"]; + } + + expect(() => { + new TestWidget(env); + }).toThrow("Missing props 'message' (widget 'TestWidget')"); + }); + + test("validate simple types", async () => { + const Tests = [ + { type: Number, ok: 1, ko: "1" }, + { type: Boolean, ok: true, ko: "1" }, + { type: String, ok: "1", ko: 1 }, + { type: Object, ok: {}, ko: "1" }, + { type: Date, ok: new Date(), ko: "1" }, + { type: Function, ok: () => {}, ko: "1" } + ]; + + for (let test of Tests) { + let TestWidget = class extends Widget { + static props = { p: test.type }; + }; + + expect(() => { + new TestWidget(env); + }).toThrow("Missing props 'p'"); + + expect(() => { + new TestWidget(env, { p: test.ok }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: test.ko }); + }).toThrow("Props 'p' of invalid type in widget"); + } + }); + + test("validate simple types, alternate form", async () => { + const Tests = [ + { type: Number, ok: 1, ko: "1" }, + { type: Boolean, ok: true, ko: "1" }, + { type: String, ok: "1", ko: 1 }, + { type: Object, ok: {}, ko: "1" }, + { type: Date, ok: new Date(), ko: "1" }, + { type: Function, ok: () => {}, ko: "1" } + ]; + + for (let test of Tests) { + let TestWidget = class extends Widget { + static props = { p: { type: test.type } }; + }; + + expect(() => { + new TestWidget(env); + }).toThrow("Missing props 'p'"); + + expect(() => { + new TestWidget(env, { p: test.ok }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: test.ko }); + }).toThrow("Props 'p' of invalid type in widget"); + } + }); + + + test("can validate a prop with multiple types", async () => { + let TestWidget = class extends Widget { + static props = { p: [String, Boolean] }; + }; + + expect(() => { + new TestWidget(env, { p: "string" }); + new TestWidget(env, { p: true }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: 1 }); + }).toThrow("Props 'p' of invalid type in widget"); + }); + + test("can validate an optional props", async () => { + let TestWidget = class extends Widget { + static props = { p: { type: String, optional: true } }; + }; + + expect(() => { + new TestWidget(env, { p: "hey" }); + new TestWidget(env, { }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: 1 }); + }).toThrow(); + + }); + + + test("can validate an array with given primitive type", async () => { + let TestWidget = class extends Widget { + static props = { p: { type: Array, element: String } }; + }; + + expect(() => { + new TestWidget(env, { p: [] }); + new TestWidget(env, { p: ["string"] }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: [1] }); + }).toThrow(); + + expect(() => { + new TestWidget(env, { p: ["string", 1] }); + }).toThrow(); + }); + + test("can validate an array with multiple sub element types", async () => { + let TestWidget = class extends Widget { + static props = { p: { type: Array, element: [String, Boolean] } }; + }; + + expect(() => { + new TestWidget(env, { p: [] }); + new TestWidget(env, { p: ["string"] }); + new TestWidget(env, { p: [false, true, "string"] }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: [true, 1] }); + }).toThrow(); + }); + + test("can validate an object with simple shape", async () => { + let TestWidget = class extends Widget { + static props = { + p: { type: Object, shape: { id: Number, url: String } } + }; + }; + + expect(() => { + new TestWidget(env, { p: { id: 1, url: "url" } }); + new TestWidget(env, { p: { id: 1, url: "url", extra: true } }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: { id: "1", url: "url" } }); + }).toThrow(); + + expect(() => { + new TestWidget(env, { p: { id: 1 } }); + }).toThrow(); + }); + + test("can validate recursively complicated prop def", async () => { + let TestWidget = class extends Widget { + static props = { + p: { + type: Object, + shape: { + id: Number, + url: [Boolean, {type: Array, element: Number}], + } + } + }; + }; + + expect(() => { + new TestWidget(env, { p: { id: 1, url: true } }); + new TestWidget(env, { p: { id: 1, url: [12]} }); + }).not.toThrow(); + + expect(() => { + new TestWidget(env, { p: { id: 1, url: [12, true]} }); + }).toThrow(); + + }); +}); + + +describe("default props", () => { + test("can set default values", async () => { + class TestWidget extends Widget { + static defaultProps = {p: 4} + } + + const w = new TestWidget(env, {}); + expect(w.props.p).toBe(4); + }); + + test("default values are also set whenever component is updated", async () => { + class TestWidget extends Widget { + static defaultProps = {p: 4} + } + + const w = new TestWidget(env, {p: 1}); + await w._updateProps({}); + expect(w.props.p).toBe(4); + }); +});