From 403a7c2eb766f33da94b23ccddc221685d2e3cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Wed, 20 Dec 2023 20:10:54 +0100 Subject: [PATCH] [FIX] mail: remove reactive from Model/store internals Internal code doesn't need to use reactive when reading store or Model, especially not in `insert` that is not meant to access anything observed except when actually writing records (in which case `_proxy` is used). The `get` part of `preinsert` (and its full chain) must be kept with the full proxy however, to ensure proper trigger of reactive callback when returned record changes. Part-of: odoo/odoo#147814 --- addons/mail/static/src/core/common/record.js | 105 +++++++++--------- addons/mail/static/tests/core/record_tests.js | 17 +++ 2 files changed, 72 insertions(+), 50 deletions(-) diff --git a/addons/mail/static/src/core/common/record.js b/addons/mail/static/src/core/common/record.js index 70dadc0b4e3..19a1b63b4f8 100644 --- a/addons/mail/static/src/core/common/record.js +++ b/addons/mail/static/src/core/common/record.js @@ -38,7 +38,7 @@ function updateFields(record, vals) { // "store[Model] =" is considered a Model.insert() record[fieldName].insert(value); } else { - const fieldDefinition = record._RawModel._fields.get(fieldName); + const fieldDefinition = record.Model._fields.get(fieldName); if (!fieldDefinition || Record.isAttr(fieldDefinition)) { updateAttr(record, fieldName, value); } else { @@ -54,7 +54,7 @@ function updateFields(record, vals) { * @param {any} value */ function updateAttr(record, fieldName, value) { - const fieldDefinition = record._RawModel._fields.get(fieldName); + const fieldDefinition = record.Model._fields.get(fieldName); // ensure each field write goes through the proxy exactly once to trigger reactives const targetRecord = record._proxyUsed.has(fieldName) ? record : record._proxy; if ( @@ -172,7 +172,7 @@ function sortRecordList(recordListFullProxy, func) { export function makeStore(env) { Record.UPDATE = 0; - const recordByLocalId = new Map(); + const recordByLocalId = reactive(new Map()); const res = { // fake store for now, until it becomes a model /** @type {import("models").Store} */ @@ -193,7 +193,7 @@ export function makeStore(env) { // work-around: make an object whose prototype is the class, so that static props become // instance props. /** @type {typeof Record} */ - const Model = Object.assign(Object.create(OgClass), { env, store: res.store }); + const Model = Object.create(OgClass); // Produce another class with changed prototype, so that there are automatic get/set on relational fields const Class = { [OgClass.name]: class extends OgClass { @@ -204,7 +204,7 @@ export function makeStore(env) { record._proxyUsed = new Set(); record._updateFields = new Set(); record._raw = record; - record._RawModel = Model; + record.Model = Model; const recordProxyInternal = new Proxy(record, { get(record, name, recordFullProxy) { recordFullProxy = record._downgradeProxy(recordFullProxy); @@ -267,7 +267,7 @@ export function makeStore(env) { const recordProxy = reactive(recordProxyInternal); record._proxy = recordProxy; if (record instanceof BaseStore) { - res.store = recordProxy; + res.store = record; } for (const [name, fieldDefinition] of Model._fields) { const SYM = record[name]?.[0]; @@ -287,11 +287,7 @@ export function makeStore(env) { owner: record, _raw: recordList, }); - if (record instanceof BaseStore) { - recordList.store = recordProxy; - } else { - recordList.store = res.store; - } + recordList.store = res.store; field.value = recordList; } else { record[name] = fieldDefinition.default; @@ -410,7 +406,8 @@ export function makeStore(env) { }[OgClass.name]; Object.assign(Model, { Class, - records: JSON.parse(JSON.stringify(OgClass.records)), + env, + records: reactive({}), _fields: new Map(), }); Models[name] = Model; @@ -454,15 +451,20 @@ export function makeStore(env) { } } } + /** + * store/_rawStore are assigned on models at next step, but they are + * required on Store model to make the initial store insert. + */ + Object.assign(res.store.Store, { store: res.store, _rawStore: res.store }); // Make true store (as a model) - res.store = res.store.Store.insert(); - const store = toRaw(res.store)._raw; + res.store = toRaw(res.store.Store.insert())._raw; for (const Model of Object.values(Models)) { - Model.store = res.store; - res.store[Model.name] = Model; + Model._rawStore = res.store; + Model.store = res.store._proxy; + res.store._proxy[Model.name] = Model; } - Object.assign(store, { Models, storeReady: true }); - return res.store; + Object.assign(res.store, { Models, storeReady: true }); + return res.store._proxy; } class RecordUses { @@ -522,7 +524,7 @@ class RecordList extends Array { data = []; get fieldDefinition() { - return this.owner._RawModel._fields.get(this.name); + return this.owner.Model._fields.get(this.name); } constructor() { @@ -572,7 +574,7 @@ class RecordList extends Array { // support for "array[index] = r3" syntax const index = parseInt(name); recordList._insert(val, function recordListSet_Insert(newRecord) { - const oldRecord = toRaw(recordList.store)._raw.recordByLocalId.get( + const oldRecord = toRaw(recordList.store.recordByLocalId).get( recordList.data[index] ); if (oldRecord && oldRecord.notEq(newRecord)) { @@ -1021,7 +1023,7 @@ export class Record { static trusted = false; static id; /** @type {Object} */ - static records = {}; + static records; /** @type {import("models").Store} */ static store; /** @type {RecordField[]} */ @@ -1108,8 +1110,9 @@ export class Record { } for (const [localId, names] of record.__uses__.data.entries()) { for (const [name2, count] of names.entries()) { - const usingRecordProxy = - record._RawModel.store.recordByLocalId.get(localId); + const usingRecordProxy = toRaw( + record.Model._rawStore.recordByLocalId + ).get(localId); if (!usingRecordProxy) { // record already deleted, clean inverses record.__uses__.data.delete(localId); @@ -1127,7 +1130,7 @@ export class Record { } } delete record.Model.records[record.localId]; - record._RawModel.store.recordByLocalId.delete(record.localId); + record.Model._rawStore.recordByLocalId.delete(record.localId); } } Record.UPDATE--; @@ -1263,7 +1266,7 @@ export class Record { * * @type {Map} */ - static _fields = new Map(); + static _fields; static isRecord(record) { return Boolean(record?.[IS_RECORD_SYM]); } @@ -1331,6 +1334,7 @@ export class Record { return res; } static _retrieveIdFromData(data) { + const Model = toRaw(this); const res = {}; function _deepRetrieve(expr2) { if (typeof expr2 === "string") { @@ -1359,18 +1363,18 @@ export class Record { } } } - if (this.id === undefined) { + if (Model.id === undefined) { return res; } - if (typeof this.id === "string") { + if (typeof Model.id === "string") { if (typeof data !== "object" || data === null) { - return { [this.id]: data }; // non-object data => single id + return { [Model.id]: data }; // non-object data => single id } - if (Record.isCommand(data[this.id])) { + if (Record.isCommand(data[Model.id])) { // Note: only Record.one() is supported - const [cmd, data2] = data[this.id].at(-1); + const [cmd, data2] = data[Model.id].at(-1); return Object.assign(res, { - [this.id]: + [Model.id]: cmd === "DELETE" ? undefined : cmd === "DELETE.noinv" @@ -1380,9 +1384,9 @@ export class Record { : data2, }); } - return { [this.id]: data[this.id] }; + return { [Model.id]: data[Model.id] }; } - for (const expr of this.id) { + for (const expr of Model.id) { if (typeof expr === "symbol") { continue; } @@ -1407,12 +1411,10 @@ export class Record { * @returns {Record} */ static new(data) { - const ModelProxy = this; - const Model = toRaw(ModelProxy); + const Model = toRaw(this); return Record.MAKE_UPDATE(function RecordNew() { const recordProxy = new Model.Class(); const record = toRaw(recordProxy)._raw; - record.Model = ModelProxy; const ids = Model._retrieveIdFromData(data); for (const name in ids) { if ( @@ -1423,21 +1425,21 @@ export class Record { ) { // preinsert that record in relational field, // as it is required to make current local id - ids[name] = Model.store[Model._fields.get(name).targetModel].preinsert( + ids[name] = Model._rawStore[Model._fields.get(name).targetModel].preinsert( ids[name] ); } } Object.assign(record, { localId: Model.localId(ids) }); Object.assign(recordProxy, { ...ids }); - ModelProxy.records[record.localId] = recordProxy; - if (record._RawModel.name === "Store") { + Model.records[record.localId] = recordProxy; + if (record.Model.name === "Store") { Object.assign(record, { - env: Model.store.env, - recordByLocalId: Model.store.recordByLocalId, + env: Model._rawStore.env, + recordByLocalId: Model._rawStore.recordByLocalId, }); } - Model.store.recordByLocalId.set(record.localId, recordProxy); + Model._rawStore.recordByLocalId.set(record.localId, recordProxy); for (const field of record._fields.values()) { field.requestCompute?.(); field.requestSort?.(); @@ -1524,7 +1526,7 @@ export class Record { } /** @returns {Record|Record[]} */ static insert(data, options = {}) { - const SubClass = this; + const ModelFullProxy = this; return Record.MAKE_UPDATE(function RecordInsert() { const isMulti = Array.isArray(data); if (!isMulti) { @@ -1533,7 +1535,7 @@ export class Record { const oldTrusted = Record.trusted; Record.trusted = options.html ?? Record.trusted; const res = data.map(function RecordInsertMap(d) { - return SubClass._insert(d, options); + return ModelFullProxy._insert(d, options); }); Record.trusted = oldTrusted; if (!isMulti) { @@ -1544,7 +1546,8 @@ export class Record { } /** @returns {Record} */ static _insert(data) { - const res = this.preinsert(data); + const ModelFullProxy = this; + const res = ModelFullProxy.preinsert(data); res.update(data); return res; } @@ -1553,7 +1556,9 @@ export class Record { * @returns {Record} */ static preinsert(data) { - return this.get(data) ?? this.new(data); + const ModelFullProxy = this; + const Model = toRaw(ModelFullProxy); + return ModelFullProxy.get(data) ?? Model.new(data); } static isCommand(data) { return ["ADD", "DELETE", "ADD.noinv", "DELETE.noinv"].includes(data?.[0]?.[0]); @@ -1569,7 +1574,7 @@ export class Record { _fields = new Map(); __uses__ = markRaw(new RecordUses()); get _store() { - return toRaw(this)._raw._RawModel.store; + return toRaw(this)._raw.Model._rawStore._proxy; } /** * Technical attribute, contains the Model entry in the store. @@ -1602,7 +1607,7 @@ export class Record { updateFields(record, data); } else { // update on single-id data - updateFields(record, { [record._RawModel.id]: data }); + updateFields(record, { [record.Model.id]: data }); } }); } @@ -1659,14 +1664,14 @@ export class Record { delete data._proxyInternal; delete data._proxyUsed; delete data._raw; - delete data._RawModel; + delete data.Model; delete data._updateFields; delete data.__uses__; delete data.Model; return data; } toIdData() { - const data = this._RawModel._retrieveIdFromData(this); + const data = this.Model._retrieveIdFromData(this); for (const [name, val] of Object.entries(data)) { if (Record.isRecord(val)) { data[name] = val.toIdData(); diff --git a/addons/mail/static/tests/core/record_tests.js b/addons/mail/static/tests/core/record_tests.js index 5bb14319085..d93cd24afd7 100644 --- a/addons/mail/static/tests/core/record_tests.js +++ b/addons/mail/static/tests/core/record_tests.js @@ -582,3 +582,20 @@ QUnit.test("lazy sort should re-sort while they are observed", async (assert) => message.sequence = 10; assert.verifySteps(["render 2,1"]); }); + +QUnit.test("store updates can be observed", async (assert) => { + const store = await start(); + function onUpdate() { + assert.step(`abc:${reactiveStore.abc}`); + } + const rawStore = toRaw(store)._raw; + const reactiveStore = reactive(store, onUpdate); + onUpdate(); + assert.verifySteps(["abc:undefined"]); + store.abc = 1; + assert.verifySteps(["abc:1"], "observable from makeStore"); + rawStore._store.abc = 2; + assert.verifySteps(["abc:2"], "observable from record._store"); + rawStore.Model.store.abc = 3; + assert.verifySteps(["abc:3"], "observable from Model.store"); +});