From 08cb83149ea28fb44a5ec3e4276329412258876d Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Wed, 30 Oct 2019 15:15:32 +0100 Subject: [PATCH] [REM] component: remove t-keepalive directive We don't see any usecase for it, it makes the code more complex, and there were still potential unresolved concurrency issues with it. Part of task #295 --- doc/reference/component.md | 5 +- doc/reference/qweb.md | 18 +-- src/component/directive.ts | 27 +--- src/store.ts | 1 - .../__snapshots__/component.test.ts.snap | 90 ------------ tests/component/component.test.ts | 133 ------------------ tools/playground/app.js | 1 - tools/playground/templates.xml | 2 +- 8 files changed, 16 insertions(+), 261 deletions(-) diff --git a/doc/reference/component.md b/doc/reference/component.md index 35f05ed4..ae99b39e 100644 --- a/doc/reference/component.md +++ b/doc/reference/component.md @@ -391,8 +391,7 @@ scrollbar. Note that modifying the state is not allowed here. This method is called just before an actual DOM patch, and is only intended to be used to save some local -DOM state. Also, it will not be called if the component is not in the DOM (this can -happen with components with `t-keepalive`). +DOM state. Also, it will not be called if the component is not in the DOM. #### `patched(snapshot)` @@ -402,7 +401,7 @@ likely via a change in its state/props or environment). This method is not called on the initial render. It is useful to interact with the DOM (for example, through an external library) whenever the component was patched. Note that this hook will not be called if the compoent is -not in the DOM (this can happen with components with `t-keepalive`). +not in the DOM. Updating the component state in this hook is possible, but not encouraged. One needs to be careful, because updates here will create an additional rendering, which in diff --git a/doc/reference/qweb.md b/doc/reference/qweb.md index b27fabbd..c687327d 100644 --- a/doc/reference/qweb.md +++ b/doc/reference/qweb.md @@ -67,15 +67,15 @@ We present here a list of all standard QWeb directives: The component system in Owl requires additional directives, to express various needs. Here is a list of all Owl specific directives: -| Name | Description | -| ------------------------------------------------------ | ----------------------------------------------------------------------------------- | -| `t-component`, `t-props`, `t-keepalive`, `t-asyncroot` | [Defining a sub component](component.md#composition) | -| `t-ref` | [Setting a reference to a dom node or a sub component](component.md#references) | -| `t-key` | [Defining a key (to help virtual dom reconciliation)](component.md#t-key-directive) | -| `t-on-*` | [Event handling](component.md#event-handling) | -| `t-transition` | [Defining an animation](animations.md#css-transitions) | -| `t-slot` | [Rendering a slot](component.md#slots) | -| `t-model` | [Form input bindings](component.md#form-input-bindings) | +| Name | Description | +| ------------------------ | ----------------------------------------------------------------------------------- | +| `t-component`, `t-props` | [Defining a sub component](component.md#composition) | +| `t-ref` | [Setting a reference to a dom node or a sub component](component.md#references) | +| `t-key` | [Defining a key (to help virtual dom reconciliation)](component.md#t-key-directive) | +| `t-on-*` | [Event handling](component.md#event-handling) | +| `t-transition` | [Defining an animation](animations.md#css-transitions) | +| `t-slot` | [Rendering a slot](component.md#slots) | +| `t-model` | [Form input bindings](component.md#form-input-bindings) | ## QWeb Engine diff --git a/src/component/directive.ts b/src/component/directive.ts index bf32a9a9..d43c953c 100644 --- a/src/component/directive.ts +++ b/src/component/directive.ts @@ -186,7 +186,7 @@ QWeb.utils.defineProxy = function defineProxy(target, source) { QWeb.addDirective({ name: "component", - extraNames: ["props", "keepalive"], + extraNames: ["props"], priority: 100, atNodeEncounter({ ctx, value, node, qweb }): boolean { ctx.addLine("//COMPONENT"); @@ -194,7 +194,6 @@ QWeb.addDirective({ ctx.rootContext.shouldDefineQWeb = true; ctx.rootContext.shouldDefineParent = true; ctx.rootContext.shouldDefineUtils = true; - let keepAlive = node.getAttribute("t-keepalive") ? true : false; let hasDynamicProps = node.getAttribute("t-props") ? true : false; // t-on- events and t-transition @@ -256,8 +255,8 @@ QWeb.addDirective({ if (transition) { transitionsInsertCode = `utils.transitionInsert(vn, '${transition}');`; } - let finalizeComponentCode = `w${componentID}.${keepAlive ? "unmount" : "destroy"}();`; - if (ref && !keepAlive) { + let finalizeComponentCode = `w${componentID}.destroy();`; + if (ref) { finalizeComponentCode += `delete context.__owl__.refs[${refKey}];`; } if (transition) { @@ -338,11 +337,6 @@ QWeb.addDirective({ `let w${componentID} = ${templateId} in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[${templateId}]] : false;` ); let shouldProxy = !ctx.parentNode; - if (keepAlive) { - ctx.addLine( - `const fiber${componentID} = Object.assign(Object.create(extra.fiber), {patchQueue: []});` - ); - } if (shouldProxy) { let id = ctx.generateID(); ctx.rootContext.rootNode = id; @@ -393,28 +387,15 @@ QWeb.addDirective({ ctx.addIf(`w${componentID}`); // need to update component - let patchQueueCode = keepAlive ? `fiber${componentID}` : "extra.fiber"; - if (keepAlive) { - // if we have t-keepalive="1", the component could be unmounted, but then - // we __updateProps is called. This is ok, but we do not want to call - // the willPatch/patched hooks of the component in this case, so we - // disable the patch queue - patchQueueCode = `w${componentID}.__owl__.isMounted ? extra.fiber : fiber${componentID}`; - } let styleCode = ""; if (tattStyle) { styleCode = `.then(()=>{if (w${componentID}.__owl__.isDestroyed) {return};w${componentID}.el.style=${tattStyle};});`; } ctx.addLine( - `w${componentID}.__updateProps(props${componentID}, ${patchQueueCode}${scopeVars && + `w${componentID}.__updateProps(props${componentID}, extra.fiber${scopeVars && ", " + scopeVars}, sibling)${styleCode};` ); ctx.addLine(`let pvnode = w${componentID}.__owl__.pvnode;`); - let keepAliveCode = ""; - if (keepAlive) { - keepAliveCode = `pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w${componentID}.el,vn.elm);vn.elm=w${componentID}.el;w${componentID}.__remount();};`; - ctx.addLine(keepAliveCode); - } if (registerCode) { ctx.addLine(registerCode); } diff --git a/src/store.ts b/src/store.ts index 3f94fbc6..53de6779 100644 --- a/src/store.ts +++ b/src/store.ts @@ -116,7 +116,6 @@ export function useStore(selector, options: SelectorOptions = {}): any { } }); onWillUpdateProps(props => { - // FIXME: only do that if not keepalive + do it in destroy in that case delete store.updateFunctions[component.__owl__.id]; result = selector(store.state, props); }); diff --git a/tests/component/__snapshots__/component.test.ts.snap b/tests/component/__snapshots__/component.test.ts.snap index e6c9b7dc..2702de41 100644 --- a/tests/component/__snapshots__/component.test.ts.snap +++ b/tests/component/__snapshots__/component.test.ts.snap @@ -234,51 +234,6 @@ exports[`class and style attributes with t-component t-att-class is properly add }" `; -exports[`composition sub components dom state with t-keepalive is preserved 1`] = ` -"function anonymous(context,extra -) { - let utils = this.constructor.utils; - let QWeb = this.constructor; - let parent = context; - let owner = context; - let sibling = null; - var h = this.h; - let c1 = [], p1 = {key:1}; - var vn1 = h('div', p1, c1); - if (context['state'].ok) { - //COMPONENT - let templateId3 = \`__4__\`; - let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false; - const fiber3 = Object.assign(Object.create(extra.fiber), {patchQueue: []}); - let props3 = {}; - if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) { - w3.destroy(); - w3 = false; - } - if (w3) { - w3.__updateProps(props3, w3.__owl__.isMounted ? extra.fiber : fiber3, undefined, undefined, sibling); - let pvnode = w3.__owl__.pvnode; - pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w3.el,vn.elm);vn.elm=w3.el;w3.__remount();}; - c1.push(pvnode); - } else { - let componentKey3 = \`InputWidget\`; - let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['InputWidget']; - if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')} - w3 = new W3(parent, props3); - parent.__owl__.cmap[templateId3] = w3.__owl__.id; - let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling); - let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.unmount();}}}); - const fiber = w3.__owl__.currentFiber; - def2.then(function () {if (w3.__owl__.isDestroyed) {return;} const vnode = fiber.vnode; pvnode.sel = vnode.sel; }); - c1.push(pvnode); - w3.__owl__.pvnode = pvnode; - } - sibling = w3.__owl__.currentFiber || sibling; - } - return vn1; -}" -`; - exports[`composition sub components with some state rendered in a loop 1`] = ` "function anonymous(context,extra ) { @@ -460,51 +415,6 @@ exports[`dynamic t-props basic use 1`] = ` }" `; -exports[`lifecycle hooks willPatch/patched hook with t-keepalive 1`] = ` -"function anonymous(context,extra -) { - let utils = this.constructor.utils; - let QWeb = this.constructor; - let parent = context; - let owner = context; - let sibling = null; - var h = this.h; - let c1 = [], p1 = {key:1}; - var vn1 = h('div', p1, c1); - if (context['state'].flag) { - //COMPONENT - let templateId3 = \`__4__\`; - let w3 = templateId3 in parent.__owl__.cmap ? parent.__owl__.children[parent.__owl__.cmap[templateId3]] : false; - const fiber3 = Object.assign(Object.create(extra.fiber), {patchQueue: []}); - let props3 = {v:context['state'].n}; - if (w3 && w3.__owl__.currentFiber && !w3.__owl__.vnode) { - w3.destroy(); - w3 = false; - } - if (w3) { - w3.__updateProps(props3, w3.__owl__.isMounted ? extra.fiber : fiber3, undefined, undefined, sibling); - let pvnode = w3.__owl__.pvnode; - pvnode.data.hook.insert = vn => {vn.elm.parentNode.replaceChild(w3.el,vn.elm);vn.elm=w3.el;w3.__remount();}; - c1.push(pvnode); - } else { - let componentKey3 = \`ChildWidget\`; - let W3 = context.constructor.components[componentKey3] || QWeb.components[componentKey3]|| context['ChildWidget']; - if (!W3) {throw new Error('Cannot find the definition of component \\"' + componentKey3 + '\\"')} - w3 = new W3(parent, props3); - parent.__owl__.cmap[templateId3] = w3.__owl__.id; - let def2 = w3.__prepare(extra.fiber, undefined, undefined, sibling); - let pvnode = h('dummy', {key: templateId3, hook: {insert(vn) { let nvn=w3.__mount(fiber, pvnode.elm);pvnode.elm=nvn.elm;},remove() {},destroy(vn) {w3.unmount();}}}); - const fiber = w3.__owl__.currentFiber; - def2.then(function () {if (w3.__owl__.isDestroyed) {return;} const vnode = fiber.vnode; pvnode.sel = vnode.sel; }); - c1.push(pvnode); - w3.__owl__.pvnode = pvnode; - } - sibling = w3.__owl__.currentFiber || sibling; - } - return vn1; -}" -`; - exports[`other directives with t-component t-on with getter as handler 1`] = ` "function anonymous(context,extra ) { diff --git a/tests/component/component.test.ts b/tests/component/component.test.ts index a62d1ff2..001f3de4 100644 --- a/tests/component/component.test.ts +++ b/tests/component/component.test.ts @@ -836,48 +836,6 @@ describe("lifecycle hooks", () => { "parent:patched" ]); }); - - test("willPatch/patched hook with t-keepalive", async () => { - // we make sure here that willPatch/patched is only called if widget is in - // dom, mounted - const steps: string[] = []; - - class ChildWidget extends Widget { - willPatch() { - steps.push("child:willPatch"); - } - patched() { - steps.push("child:patched"); - } - willUnmount() { - steps.push("child:willUnmount"); - } - mounted() { - steps.push("child:mounted"); - } - } - class ParentWidget extends Widget { - static template = xml` -
- -
- `; - static components = { ChildWidget }; - state = useState({ n: 1, flag: true }); - } - - const widget = new ParentWidget(); - await widget.mount(fixture); - - expect(env.qweb.templates[ParentWidget.template].fn.toString()).toMatchSnapshot(); - expect(steps).toEqual(["child:mounted"]); - widget.state.flag = false; - await nextTick(); - expect(steps).toEqual(["child:mounted", "child:willUnmount"]); - widget.state.flag = true; - await nextTick(); - expect(steps).toEqual(["child:mounted", "child:willUnmount", "child:mounted"]); - }); }); describe("destroy method", () => { @@ -1287,97 +1245,6 @@ describe("composition", () => { expect(fixture.innerHTML).toBe("
0
"); }); - test("sub components with t-keepalive are not destroyed if no longer in dom", async () => { - env.qweb.addTemplate( - "ParentWidget", - `
` - ); - class ParentWidget extends Widget { - state = useState({ ok: true }); - static components = { Counter }; - } - const widget = new ParentWidget(); - await widget.mount(fixture); - const button = fixture.getElementsByTagName("button")[0]; - await button.click(); - await nextTick(); - expect(fixture.innerHTML).toBe("
1
"); - const counter = children(widget)[0]; - expect(counter.__owl__.isMounted).toBe(true); - - widget.state.ok = false; - await nextTick(); - expect(fixture.innerHTML).toBe("
"); - expect(counter.__owl__.isMounted).toBe(false); - - widget.state.ok = true; - await nextTick(); - expect(counter.__owl__.isMounted).toBe(true); - expect(fixture.innerHTML).toBe("
1
"); - }); - - test("sub components dom state with t-keepalive is preserved", async () => { - env.qweb.addTemplate( - "ParentWidget", - `
` - ); - class InputWidget extends Widget {} - class ParentWidget extends Widget { - state = useState({ ok: true }); - static components = { InputWidget }; - } - env.qweb.addTemplate("InputWidget", ""); - const widget = new ParentWidget(); - await widget.mount(fixture); - const input = fixture.getElementsByTagName("input")[0]; - input.value = "test"; - widget.state.ok = false; - await nextTick(); - expect(fixture.innerHTML).toBe("
"); - - widget.state.ok = true; - await nextTick(); - expect(fixture.innerHTML).toBe("
"); - const input2 = fixture.getElementsByTagName("input")[0]; - expect(input).toBe(input2); - expect(input2.value).toBe("test"); - expect(env.qweb.templates.ParentWidget.fn.toString()).toMatchSnapshot(); - }); - - test("sub widget with t-ref and t-keepalive", async () => { - class ChildWidget extends Widget { - static template = xml`Hello`; - } - class ParentWidget extends Widget { - static template = xml` -
- -
- `; - static components = { ChildWidget }; - state = useState({ ok: true }); - child = useRef("child"); - } - const widget = new ParentWidget(); - await widget.mount(fixture); - let child = children(widget)[0]; - - expect(fixture.innerHTML).toBe("
Hello
"); - expect(widget.child.comp).toEqual(child); - - widget.state.ok = false; - await nextTick(); - - expect(fixture.innerHTML).toBe("
"); - expect(widget.child.comp).toEqual(child); - - widget.state.ok = true; - await nextTick(); - - expect(fixture.innerHTML).toBe("
Hello
"); - expect(widget.child.comp).toEqual(child); - }); - test("sub components rendered in a loop", async () => { env.qweb.addTemplate("ChildWidget", ``); class ChildWidget extends Widget {} diff --git a/tools/playground/app.js b/tools/playground/app.js index e5d1535c..70267071 100644 --- a/tools/playground/app.js +++ b/tools/playground/app.js @@ -443,7 +443,6 @@ async function start() { owl.utils.whenReady() ]); const qweb = new owl.QWeb({ templates }); - debugger; owl.config.env = { qweb }; const app = new App(); app.mount(document.body); diff --git a/tools/playground/templates.xml b/tools/playground/templates.xml index 445fa13f..b42bd715 100644 --- a/tools/playground/templates.xml +++ b/tools/playground/templates.xml @@ -31,7 +31,7 @@ t-att-style="topEditorStyle"/>
-