Compare commits

...

6 Commits

Author SHA1 Message Date
Géry Debongnie 1291f1f175 [REL] v2.0.6
# v2.0.6

 - [IMP] devtools: provide access to Fiber and RootFiber
 - Bump shelljs and git-rev-sync
 - [FIX] props validation: do not subscribe to props keys
 - [FIX] reactivity: only show key in subscription if observed by callback
 - [FIX] components: stop rendering stale t-component when delayed
2023-02-17 14:31:18 +01:00
Géry Debongnie 6c0a3525c8 [IMP] devtools: provide access to Fiber and RootFiber
The devtools extension needs to hook itself into some internal functions of
owl, and to do that, it needs a reference to Fiber and RootFiber
classes.
2023-02-17 14:20:03 +01:00
dependabot[bot] 13241422e9 Bump shelljs and git-rev-sync
Bumps [shelljs](https://github.com/shelljs/shelljs) to 0.8.5 and updates ancestor dependency [git-rev-sync](https://github.com/kurttheviking/git-rev-sync-js). These dependencies need to be updated together.


Updates `shelljs` from 0.7.7 to 0.8.5
- [Release notes](https://github.com/shelljs/shelljs/releases)
- [Changelog](https://github.com/shelljs/shelljs/blob/master/CHANGELOG.md)
- [Commits](https://github.com/shelljs/shelljs/compare/v0.7.7...v0.8.5)

Updates `git-rev-sync` from 1.12.0 to 3.0.2
- [Release notes](https://github.com/kurttheviking/git-rev-sync-js/releases)
- [Commits](https://github.com/kurttheviking/git-rev-sync-js/compare/v1.12.0...v3.0.2)

---
updated-dependencies:
- dependency-name: shelljs
  dependency-type: indirect
- dependency-name: git-rev-sync
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
2023-02-17 11:42:38 +01:00
Géry Debongnie 8f2c7f24d7 [FIX] props validation: do not subscribe to props keys
In dev mode, owl inserts an additional props validation step. Before
this commit, the validation would iterate on all key/value pairs of each
props. If the props is reactive, this has the unfortunate side effect of
subscribing the component to each of the keys, even though it should not
be.

This commit avoids the issue by only validating the raw object.
2023-02-09 13:34:07 +01:00
Samuel Degueldre fed9cc467f [FIX] reactivity: only show key in subscription if observed by callback
Previously, the getSubscriptions debugging utility returned all observed
keys for a given target, instead of only the keys observed by the
callback it received as argument. This commit fixes that issues.
2023-02-09 10:13:41 +01:00
Samuel Degueldre 33174b301b [FIX] components: stop rendering stale t-component when delayed
Previously, if the value of a t-component directive changed, but there
was already a scheduled render for the component before that change, the
rendering of the existing component would get delayed (as it should),
but after the parent's rendering was complete, the old component which
will be destroyed during the patch would still get rendered, despite
being stale. This can cause crashes if that component relies on state
that no longer exists.

This was caused by the fact that when rendering, we check the parent
chain for an active render, and also check that the component still
exists in the parent's fiber childrenMap (ie, we know that the upcoming
patch is not about to destroy the component). To do this, we use the
component's parentKey, but in the case of t-component, the parentKey for
both possible components is the same, causing the stale component to
incorrectly believe that it's not about to be destroyed, by finding the
next component in the childrenMap under the same key.

This commit fixes that by prepending the component's name to the
parentKey, meaning that if the value of the t-component changes, so will
the key, and the render will be further delayed until the new state is
patched, at which point the stale component will be destroyed and the
following attempt to render will be cancelled as expected.

The choice to use the component's name is to simplify the
implementation, this means that if using t-component and switching
between components that have the same class name, this protection will
not work. The alternative would be to generate a unique id for the
component class, eg with a WeakMap, but this both complicates the
implementation and adds runtime overhead, for a case that is likely
extremely rare. We are open to refine the implementation should this
problem occur in practice.
2023-02-06 11:11:42 +01:00
15 changed files with 265 additions and 31 deletions
+11 -11
View File
@@ -2481,14 +2481,14 @@
}
},
"git-rev-sync": {
"version": "1.12.0",
"resolved": "https://registry.npmjs.org/git-rev-sync/-/git-rev-sync-1.12.0.tgz",
"integrity": "sha512-LAeWoK54irAVyq/dHjkq13bYw8vsItGVCgZZbFFJv256DIK+VkLqXBjVvwtTgVWegOy7JVwD+w2uk63x+iLB6g==",
"version": "3.0.2",
"resolved": "https://registry.npmjs.org/git-rev-sync/-/git-rev-sync-3.0.2.tgz",
"integrity": "sha512-Nd5RiYpyncjLv0j6IONy0lGzAqdRXUaBctuGBbrEA2m6Bn4iDrN/9MeQTXuiquw8AEKL9D2BW0nw5m/lQvxqnQ==",
"dev": true,
"requires": {
"escape-string-regexp": "1.0.5",
"graceful-fs": "4.1.11",
"shelljs": "0.7.7"
"graceful-fs": "4.1.15",
"shelljs": "0.8.5"
},
"dependencies": {
"escape-string-regexp": {
@@ -2498,9 +2498,9 @@
"dev": true
},
"graceful-fs": {
"version": "4.1.11",
"resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.1.11.tgz",
"integrity": "sha512-9x6DLUuW+ROFdMTII9ec9t/FK8va6kYcC8/LggumssLM8kNv7IdFl3VrNUqgir2tJuBVxBga1QBoRziZacO5Zg==",
"version": "4.1.15",
"resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.1.15.tgz",
"integrity": "sha512-6uHUhOPEBgQ24HM+r6b/QwWfZq+yiFcipKFrOFiBEnWdy5sdzYoi+pJeQaPI5qOLRFqWmAXUPQNsielzdLoecA==",
"dev": true
}
}
@@ -5017,9 +5017,9 @@
"dev": true
},
"shelljs": {
"version": "0.7.7",
"resolved": "https://registry.npmjs.org/shelljs/-/shelljs-0.7.7.tgz",
"integrity": "sha512-5ZXTlakejjdxXAnFl23pgPDzCcyPoshqMVWYqMH8HiP1R+i4auEKHabljL6XQlhQV58jkSRTR33Fq7OlxyLLTg==",
"version": "0.8.5",
"resolved": "https://registry.npmjs.org/shelljs/-/shelljs-0.8.5.tgz",
"integrity": "sha512-TiwcRcrkhHvbrZbnRcFYMLl30Dfov3HKqzp5tO5b4pt6G/SezKcYhmDg15zXVBswHmctSAQKznqNW2LO5tTDow==",
"dev": true,
"requires": {
"glob": "^7.0.0",
+2 -2
View File
@@ -1,6 +1,6 @@
{
"name": "@odoo/owl",
"version": "2.0.5",
"version": "2.0.6",
"description": "Odoo Web Library (OWL)",
"main": "dist/owl.cjs.js",
"module": "dist/owl.es.js",
@@ -48,7 +48,7 @@
"chalk": "^3.0.0",
"current-git-branch": "^1.1.0",
"eslint": "8.31.0",
"git-rev-sync": "^1.12.0",
"git-rev-sync": "^3.0.2",
"github-api": "^3.3.0",
"jest": "^27.1.0",
"jest-diff": "^27.3.1",
+7
View File
@@ -1223,6 +1223,13 @@ export class CodeGenerator {
})`,
});
if (ast.isDynamic) {
// If the component class changes, this can cause delayed renders to go
// through if the key doesn't change. Use the component name for now.
// This means that two component classes with the same name isn't supported
// in t-component. We can generate a unique id per class later if needed.
keyArg = `(${expr}).name + ${keyArg}`;
}
let blockExpr = `${id}(${propString}, ${keyArg}, node, this, ${ast.isDynamic ? expr : null})`;
if (ast.isDynamic) {
blockExpr = `toggler(${expr}, ${blockExpr})`;
+5 -1
View File
@@ -1,7 +1,7 @@
import { Component, ComponentConstructor, Props } from "./component";
import { ComponentNode } from "./component_node";
import { nodeErrorHandlers, OwlError, handleError } from "./error_handling";
import { Fiber, MountOptions } from "./fibers";
import { Fiber, RootFiber, MountOptions } from "./fibers";
import { Scheduler } from "./scheduler";
import { validateProps } from "./template_helpers";
import { TemplateSet, TemplateSetConfig } from "./template_set";
@@ -35,12 +35,16 @@ declare global {
interface Window {
__OWL_DEVTOOLS__: {
apps: Set<App>;
Fiber: typeof Fiber;
RootFiber: typeof RootFiber;
};
}
}
window.__OWL_DEVTOOLS__ ||= {
apps: new Set<App>(),
Fiber: Fiber,
RootFiber: RootFiber,
};
export class App<
+9 -4
View File
@@ -162,10 +162,15 @@ export function getSubscriptions(callback: Callback) {
const targets = callbacksToTargets.get(callback) || [];
return [...targets].map((target) => {
const keysToCallbacks = targetToKeysToCallbacks.get(target);
return {
target,
keys: keysToCallbacks ? [...keysToCallbacks.keys()] : [],
};
let keys = [];
if (keysToCallbacks) {
for (const [key, cbs] of keysToCallbacks) {
if (cbs.has(callback)) {
keys.push(key);
}
}
}
return { target, keys };
});
}
// Maps reactive objects to the underlying target
+2
View File
@@ -1,4 +1,5 @@
import { OwlError } from "./error_handling";
import { toRaw } from "./reactivity";
type BaseType =
| typeof String
@@ -84,6 +85,7 @@ export function validateSchema(obj: { [key: string]: any }, schema: Schema): str
if (Array.isArray(schema)) {
schema = toSchema(schema);
}
obj = toRaw(obj);
let errors = [];
// check if each value in obj has correct shape
for (let key in obj) {
@@ -8,7 +8,7 @@ exports[`basics GrandChild display is controlled by its GrandParent 1`] = `
return function template(ctx, node, key = \\"\\") {
const Comp1 = ctx['myComp'];
return toggler(Comp1, comp1({displayGrandChild: ctx['displayGrandChild']}, key + \`__1\`, node, this, Comp1));
return toggler(Comp1, comp1({displayGrandChild: ctx['displayGrandChild']}, (Comp1).name + key + \`__1\`, node, this, Comp1));
}
}"
`;
@@ -1213,6 +1213,45 @@ exports[`delayed fiber does not get rendered if it was cancelled 4`] = `
}"
`;
exports[`delayed render does not go through when t-component value changed 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
const comp1 = app.createComponent(null, false, false, false, true);
return function template(ctx, node, key = \\"\\") {
const b2 = text(\`A\`);
const Comp1 = ctx['state'].component;
const b3 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
return multi([b2, b3]);
}
}"
`;
exports[`delayed render does not go through when t-component value changed 2`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
const b2 = text(\`B\`);
const b3 = text(ctx['state'].val);
return multi([b2, b3]);
}
}"
`;
exports[`delayed render does not go through when t-component value changed 3`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
return text(\`C\`);
}
}"
`;
exports[`delayed rendering, but then initial rendering is cancelled by yet another render 1`] = `
"function anonymous(app, bdom, helpers
) {
@@ -1776,7 +1815,7 @@ exports[`t-foreach with dynamic async component 1`] = `
let b3;
if (ctx['arr']) {
const Comp1 = ctx['myComp'];
b3 = toggler(Comp1, comp1({key: ctx['arr'][0]}, key + \`__1__\${key1}\`, node, this, Comp1));
b3 = toggler(Comp1, comp1({key: ctx['arr'][0]}, (Comp1).name + key + \`__1__\${key1}\`, node, this, Comp1));
}
c_block1[i1] = withKey(multi([b3]), key1);
}
@@ -1810,7 +1849,7 @@ exports[`t-key on dom node having a component 1`] = `
return function template(ctx, node, key = \\"\\") {
const tKey_1 = ctx['key'];
const Comp1 = ctx['myComp'];
const b2 = toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, tKey_1 + key + \`__1\`, node, this, Comp1)));
const b2 = toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
return toggler(tKey_1, block1([], [b2]));
}
}"
@@ -1836,7 +1875,7 @@ exports[`t-key on dynamic async component (toggler is never patched) 1`] = `
return function template(ctx, node, key = \\"\\") {
const tKey_1 = ctx['key'];
const Comp1 = ctx['myComp'];
return toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, tKey_1 + key + \`__1\`, node, this, Comp1)));
return toggler(tKey_1, toggler(Comp1, comp1({key: ctx['key']}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
}
}"
`;
@@ -931,7 +931,7 @@ exports[`can catch errors catching error, rethrow, render parent -- a main comp
function slot1(ctx, node, key = \\"\\") {
const Comp1 = ctx['cp'].Comp;
return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
}
return function template(ctx, node, key = \\"\\") {
@@ -1010,7 +1010,7 @@ exports[`can catch errors catching in child makes parent render 1`] = `
function slot1(ctx, node, key = \\"\\") {
const Comp1 = ctx['elem'][1];
return toggler(Comp1, comp1({id: ctx['elem'][0]}, key + \`__1\`, node, this, Comp1));
return toggler(Comp1, comp1({id: ctx['elem'][0]}, (Comp1).name + key + \`__1\`, node, this, Comp1));
}
return function template(ctx, node, key = \\"\\") {
@@ -841,6 +841,33 @@ exports[`props validation props are validated whenever component is updated 2`]
}"
`;
exports[`props validation props validation does not cause additional subscription 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
const comp1 = app.createComponent(\`Child\`, true, false, false, false);
return function template(ctx, node, key = \\"\\") {
const props1 = {obj: ctx['obj']};
helpers.validateProps(\`Child\`, props1, this);
const b2 = comp1(props1, key + \`__1\`, node, this, null);
const b3 = text(ctx['obj'].otherValue);
return multi([b2, b3]);
}
}"
`;
exports[`props validation props validation does not cause additional subscription 2`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
return text(ctx['props'].obj.value);
}
}"
`;
exports[`props validation props: list of strings 1`] = `
"function anonymous(app, bdom, helpers
) {
@@ -140,3 +140,39 @@ exports[`reactivity in lifecycle state changes in willUnmount do not trigger rer
}
}"
`;
exports[`subscriptions subscriptions returns the keys and targets observed by the component 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
return text(ctx['state'].a);
}
}"
`;
exports[`subscriptions subscriptions returns the keys observed by the component 1`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
const comp1 = app.createComponent(\`Child\`, true, false, false, false);
return function template(ctx, node, key = \\"\\") {
const b2 = text(ctx['state'].a);
const b3 = comp1({state: ctx['state']}, key + \`__1\`, node, this, null);
return multi([b2, b3]);
}
}"
`;
exports[`subscriptions subscriptions returns the keys observed by the component 2`] = `
"function anonymous(app, bdom, helpers
) {
let { text, createBlock, list, multi, html, toggler, comment } = bdom;
return function template(ctx, node, key = \\"\\") {
return text(ctx['props'].state.b);
}
}"
`;
@@ -10,7 +10,7 @@ exports[`t-component can switch between dynamic components without the need for
return function template(ctx, node, key = \\"\\") {
const Comp1 = ctx['constructor'].components[ctx['state'].child];
const b2 = toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
const b2 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
return block1([], [b2]);
}
}"
@@ -51,7 +51,7 @@ exports[`t-component can use dynamic components (the class) if given (with diffe
return function template(ctx, node, key = \\"\\") {
const tKey_1 = ctx['state'].child;
const Comp1 = ctx['myComponent'];
return toggler(tKey_1, toggler(Comp1, comp1({}, tKey_1 + key + \`__1\`, node, this, Comp1)));
return toggler(tKey_1, toggler(Comp1, comp1({}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
}
}"
`;
@@ -91,7 +91,7 @@ exports[`t-component can use dynamic components (the class) if given 1`] = `
return function template(ctx, node, key = \\"\\") {
const tKey_1 = ctx['state'].child;
const Comp1 = ctx['myComponent'];
return toggler(tKey_1, toggler(Comp1, comp1({}, tKey_1 + key + \`__1\`, node, this, Comp1)));
return toggler(tKey_1, toggler(Comp1, comp1({}, (Comp1).name + tKey_1 + key + \`__1\`, node, this, Comp1)));
}
}"
`;
@@ -132,7 +132,7 @@ exports[`t-component modifying a sub widget 1`] = `
return function template(ctx, node, key = \\"\\") {
const Comp1 = ctx['Counter'];
const b2 = toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
const b2 = toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
return block1([], [b2]);
}
}"
@@ -162,7 +162,7 @@ exports[`t-component switching dynamic component 1`] = `
return function template(ctx, node, key = \\"\\") {
const Comp1 = ctx['Child'];
return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
}
}"
`;
@@ -199,7 +199,7 @@ exports[`t-component t-component works in simple case 1`] = `
return function template(ctx, node, key = \\"\\") {
const Comp1 = ctx['Child'];
return toggler(Comp1, comp1({}, key + \`__1\`, node, this, Comp1));
return toggler(Comp1, comp1({}, (Comp1).name + key + \`__1\`, node, this, Comp1));
}
}"
`;
+59
View File
@@ -1,6 +1,7 @@
import {
App,
Component,
ComponentConstructor,
mount,
onMounted,
onRendered,
@@ -4067,6 +4068,64 @@ test("renderings, destruction, patch, stuff, ... yet another variation", async (
expect(fixture.innerHTML).toBe("ABD<p>2</p>");
});
test("delayed render does not go through when t-component value changed", async () => {
class C extends Component {
static template = xml`C`;
setup() {
useLogLifecycle("", true);
}
}
class B extends Component {
static template = xml`B<t t-esc="state.val"/>`;
state = useState({ val: 1 });
setup() {
useLogLifecycle("", true);
b = this;
}
}
let b: B;
class A extends Component {
static template = xml`A<t t-component="state.component"/>`;
state: { component: ComponentConstructor } = useState({ component: B });
setup() {
useLogLifecycle("", true);
}
}
const a = await mount(A, fixture);
expect(fixture.innerHTML).toBe("AB1");
expect([
"A:setup",
"A:willRender",
"B:setup",
"A:rendered",
"B:willRender",
"B:rendered",
"B:mounted",
"A:mounted",
]).toBeLogged();
// start a render in B
b!.state.val = 2;
// start a render in A, invalidating the scheduled render of B, which could crash if executed.
a.state.component = C;
await nextTick();
expect(fixture.innerHTML).toBe("AC");
expect([
"A:willRender",
"C:setup",
"A:rendered",
"C:willRender",
"C:rendered",
"A:willPatch",
"B:willUnmount",
"B:willDestroy",
"C:mounted",
"A:patched",
]).toBeLogged();
});
// test.skip("components with shouldUpdate=false", async () => {
// const state = { p: 1, cc: 10 };
+24 -1
View File
@@ -1,5 +1,5 @@
import { makeTestFixture, nextAppError, nextTick, snapshotEverything } from "../helpers";
import { Component, onError, xml, mount, OwlError } from "../../src";
import { Component, onError, xml, mount, OwlError, useState } from "../../src";
import { App, DEV_MSG } from "../../src/runtime/app";
import { validateProps } from "../../src/runtime/template_helpers";
import { Schema } from "../../src/runtime/validation";
@@ -682,6 +682,29 @@ describe("props validation", () => {
expect(error!.message).toBe("Invalid props for component 'SubComp': 'p' is missing");
});
test("props validation does not cause additional subscription", async () => {
let obj = {
value: 1,
otherValue: 2,
};
class Child extends Component {
static props = {
obj: { type: Object, shape: { value: Number, otherValue: Number } },
};
static template = xml`<t t-esc="props.obj.value"/>`;
}
class Parent extends Component {
static template = xml`<Child obj="obj"/><t t-esc="obj.otherValue"/>`;
static components = { Child };
obj = useState(obj);
}
const app = new App(Parent, { test: true });
await app.mount(fixture);
expect(fixture.innerHTML).toBe("12");
expect(app.root!.subscriptions).toEqual([{ keys: ["otherValue"], target: obj }]);
});
test("props are validated whenever component is updated", async () => {
let error: Error;
class SubComp extends Component {
+32
View File
@@ -7,6 +7,7 @@ import {
onWillUnmount,
useState,
xml,
toRaw,
} from "../../src";
import { makeTestFixture, nextTick, snapshotEverything, useLogLifecycle } from "../helpers";
@@ -232,3 +233,34 @@ describe("reactivity in lifecycle", () => {
expect(fixture.innerHTML).toBe("34");
});
});
describe("subscriptions", () => {
test("subscriptions returns the keys and targets observed by the component", async () => {
class Comp extends Component {
static template = xml`<t t-esc="state.a"/>`;
state = useState({ a: 1, b: 2 });
}
const comp = await mount(Comp, fixture);
expect(fixture.innerHTML).toBe("1");
expect(comp.__owl__.subscriptions).toEqual([{ keys: ["a"], target: toRaw(comp.state) }]);
});
test("subscriptions returns the keys observed by the component", async () => {
class Child extends Component {
static template = xml`<t t-esc="props.state.b"/>`;
setup() {
child = this;
}
}
let child: Child;
class Parent extends Component {
static template = xml`<t t-esc="state.a"/><Child state="state"/>`;
static components = { Child };
state = useState({ a: 1, b: 2 });
}
const parent = await mount(Parent, fixture);
expect(fixture.innerHTML).toBe("12");
expect(parent.__owl__.subscriptions).toEqual([{ keys: ["a"], target: toRaw(parent.state) }]);
expect(child!.__owl__.subscriptions).toEqual([{ keys: ["b"], target: toRaw(parent.state) }]);
});
});