From a4021152276370035dae83f633f5bbae58e5face Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 4 Feb 2022 09:36:54 +0100 Subject: [PATCH] [IMP] props validation: cannot set default value on mandatory props --- CHANGELOG.md | 2 ++ doc/reference/props.md | 7 ++++- src/component/props_validation.ts | 15 ++++++++-- .../props_validation.test.ts.snap | 17 +++++++++-- tests/components/props_validation.test.ts | 30 +++++++++++++++++-- 5 files changed, 63 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 639f4de1..dfa57c04 100644 --- a/CHANGELOG.md +++ b/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** diff --git a/doc/reference/props.md b/doc/reference/props.md index bcbec298..50ed0b80 100644 --- a/doc/reference/props.md +++ b/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 } }; ... diff --git a/src/component/props_validation.ts b/src/component/props_validation.ts index 0e7b5594..35c1ffe9 100644 --- a/src/component/props_validation.ts +++ b/src/component/props_validation.ts @@ -47,6 +47,7 @@ export function validateProps

(name: string | ComponentConstructor

, props: } applyDefaultProps(props, ComponentClass); + const defaultProps = ComponentClass.defaultProps || {}; let propsDef = getPropDescription(ComponentClass.props); const allowAdditionalProps = "*" in propsDef; @@ -54,8 +55,18 @@ export function validateProps

(name: string | ComponentConstructor

, 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

(name: string | ComponentConstructor

, 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 diff --git a/tests/components/__snapshots__/props_validation.test.ts.snap b/tests/components/__snapshots__/props_validation.test.ts.snap index 47803895..805885f8 100644 --- a/tests/components/__snapshots__/props_validation.test.ts.snap +++ b/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; diff --git a/tests/components/props_validation.test.ts b/tests/components/props_validation.test.ts index 40300138..c808d685 100644 --- a/tests/components/props_validation.test.ts +++ b/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`

`; static defaultProps = { p: 4 }; } @@ -791,9 +791,9 @@ describe("default props", () => { expect(fixture.innerHTML).toBe("
4
"); }); - 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`heyhey`; } @@ -804,4 +804,28 @@ describe("default props", () => { await mount(Parent, fixture, { dev: true }); expect(fixture.innerHTML).toBe("
heyhey
"); }); + + 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`
`; + } + class Parent extends Component { + static components = { Child }; + static template = xml``; + } + 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" + ); + }); });