From d5bab8cc6d39bf492d7bf37b995edcb995f1970b Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 30 Aug 2016 19:14:16 +0200 Subject: [PATCH] Added support for setters on computed props, fixes #421, #463 --- CHANGELOG.md | 1 + src/api/computeddecorator.ts | 27 +++++++++++---------- src/api/createtransformer.ts | 2 +- src/api/observabledecorator.ts | 2 +- src/core/computedvalue.ts | 14 +++++++---- src/types/observableobject.ts | 29 +++++++++++++++-------- test/babel/babel-tests.js | 19 +++++++++++++++ test/observables.js | 43 +++++++++++++++++++++++++++++++++- test/typescript-tests.ts | 25 ++++++++++++-------- 9 files changed, 123 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2c416398..52dc312b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # 2.5.0 +* Introduced setters for computed properties, use `computed(expr, setter)` or `@computed name get() { return expr } set(value) { action }`, #421, #463 * Introduced `isStrictModeEnabled()`, deprecated `useStrict()` without arguments, see #464 * Fixed #505, accessing an observable property throws before it is initialized diff --git a/src/api/computeddecorator.ts b/src/api/computeddecorator.ts index 25b10a16d..5e7c21533 100644 --- a/src/api/computeddecorator.ts +++ b/src/api/computeddecorator.ts @@ -13,6 +13,7 @@ const computedDecorator = createClassPropertyDecorator( (target, name, _, decoratorArgs, originalDescriptor) => { invariant(typeof originalDescriptor !== "undefined", "@computed can only be used on getter functions, like: '@computed get myProps() { return ...; }'. It looks like it was used on a property."); const baseValue = originalDescriptor.get; + const setter = originalDescriptor.set; invariant(typeof baseValue === "function", "@computed can only be used on getter functions, like: '@computed get myProps() { return ...; }'"); let compareStructural = false; @@ -20,7 +21,7 @@ const computedDecorator = createClassPropertyDecorator( compareStructural = true; const adm = asObservableObject(target, undefined, ValueMode.Recursive); - defineObservableProperty(adm, name, compareStructural ? asStructure(baseValue) : baseValue, false); + defineObservableProperty(adm, name, compareStructural ? asStructure(baseValue) : baseValue, false, setter); }, function (name) { const observable = this.$mobx.values[name]; @@ -28,7 +29,9 @@ const computedDecorator = createClassPropertyDecorator( return undefined; return observable.get(); }, - throwingComputedValueSetter, + function (name, value) { + this.$mobx.values[name].set(value); + }, false, true ); @@ -37,21 +40,21 @@ const computedDecorator = createClassPropertyDecorator( * Decorator for class properties: @computed get value() { return expr; }. * For legacy purposes also invokable as ES5 observable created: `computed(() => expr)`; */ +export function computed(func: () => T, setter: (value: T) => void): IComputedValue; export function computed(func: () => T, scope?: any): IComputedValue; export function computed(opts: IComputedValueOptions): (target: Object, key: string, baseDescriptor?: PropertyDescriptor) => void; export function computed(target: Object, key: string | symbol, baseDescriptor?: PropertyDescriptor): void; -export function computed(targetOrExpr: any, keyOrScope?: any, baseDescriptor?: PropertyDescriptor, options?: IComputedValueOptions) { - if (arguments.length < 3 && typeof targetOrExpr === "function") - return computedExpr(targetOrExpr, keyOrScope); - invariant(!baseDescriptor || !baseDescriptor.set, `@observable properties cannot have a setter: ${keyOrScope}`); +export function computed(targetOrExpr: any, keyOrScopeOrSetter?: any, baseDescriptor?: PropertyDescriptor, options?: IComputedValueOptions) { + if (typeof targetOrExpr === "function" && arguments.length < 3) { + if (typeof keyOrScopeOrSetter === "function") + return computedExpr(targetOrExpr, keyOrScopeOrSetter, undefined); + else + return computedExpr(targetOrExpr, undefined, keyOrScopeOrSetter); + } return computedDecorator.apply(null, arguments); } -function computedExpr(expr: () => T, scope?: any) { +function computedExpr(expr: () => T, setter, scope: any) { const [mode, value] = getValueModeFromValue(expr, ValueMode.Recursive); - return new ComputedValue(value, scope, mode === ValueMode.Structure, value.name); -} - -export function throwingComputedValueSetter() { - throw new Error(`[ComputedValue] It is not allowed to assign new values to computed properties.`); + return new ComputedValue(value, scope, mode === ValueMode.Structure, value.name, setter); } diff --git a/src/api/createtransformer.ts b/src/api/createtransformer.ts index 32c4f2d49..87a5aa531 100644 --- a/src/api/createtransformer.ts +++ b/src/api/createtransformer.ts @@ -17,7 +17,7 @@ export function createTransformer(transformer: ITransformer, onClean // Local transformer class specifically for this transformer class Transformer extends ComputedValue { constructor(private sourceIdentifier: string, private sourceObject: A) { - super(() => transformer(sourceObject), null, false, `Transformer-${(transformer).name}-${sourceIdentifier}`); + super(() => transformer(sourceObject), null, false, `Transformer-${(transformer).name}-${sourceIdentifier}`, undefined); } onBecomeUnobserved() { const lastValue = this.value; diff --git a/src/api/observabledecorator.ts b/src/api/observabledecorator.ts index c84aa9dcd..aca01729a 100644 --- a/src/api/observabledecorator.ts +++ b/src/api/observabledecorator.ts @@ -11,7 +11,7 @@ const decoratorImpl = createClassPropertyDecorator( if (typeof baseValue === "function") baseValue = asReference(baseValue); const adm = asObservableObject(target, undefined, ValueMode.Recursive); - defineObservableProperty(adm, name, baseValue, true); + defineObservableProperty(adm, name, baseValue, true, undefined); allowStateChangesEnd(prevA); }, function (name) { diff --git a/src/core/computedvalue.ts b/src/core/computedvalue.ts index 85d64c7db..aec2b6c60 100644 --- a/src/core/computedvalue.ts +++ b/src/core/computedvalue.ts @@ -1,7 +1,7 @@ import {IObservable, reportObserved} from "./observable"; import {IDerivation, trackDerivedFunction, isComputingDerivation, clearObserving, untrackedStart, untrackedEnd} from "./derivation"; import {globalState} from "./globalstate"; -import {allowStateChangesStart, allowStateChangesEnd} from "./action"; +import {allowStateChangesStart, allowStateChangesEnd, createAction} from "./action"; import {getNextId, valueDidChange, invariant, Lambda, unique, joinStrings} from "../utils/utils"; import {isSpyEnabled, spyReport} from "../core/spy"; import {autorun} from "../api/autorun"; @@ -34,6 +34,7 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva dependencyStaleCount = 0; // nr of nodes being observed that are currently not ready protected value: T = undefined; name: string; + setter: (value: T) => void; /** * Create a new computed value based on a function expression. @@ -45,8 +46,10 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva * However, enabling compareStructural can be convienent if you always produce an new aggregated object and don't want to notify observers if it is structurally the same. * This is useful for working with vectors, mouse coordinates etc. */ - constructor(public derivation: () => T, private scope: Object, private compareStructural: boolean, name: string) { + constructor(public derivation: () => T, private scope: Object, private compareStructural: boolean, name: string, setter: (v: T) => void) { this.name = name || "ComputedValue@" + getNextId(); + if (setter) + this.setter = createAction(name + "-setter", setter) as any; } peek() { @@ -100,8 +103,11 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva return this.value; } - public set(_: T) { - throw new Error(`[ComputedValue '${name}'] It is not possible to assign a new value to a computed value.`); + public set(value: T) { + if (this.setter) + this.setter.call(this.scope, value); + else + throw new Error(`[ComputedValue '${this.name}'] It is not possible to assign a new value to a computed value.`); } private trackAndCompute(): boolean { diff --git a/src/types/observableobject.ts b/src/types/observableobject.ts index 30432a8cd..e7451fe3b 100644 --- a/src/types/observableobject.ts +++ b/src/types/observableobject.ts @@ -4,7 +4,6 @@ import {isAction} from "../api/action"; import {ValueMode, AsStructure} from "./modifiers"; import {Lambda, getNextId, invariant, assertPropertyConfigurable, isPlainObject, addHiddenFinalProp} from "../utils/utils"; import {runLazyInitializers} from "../utils/decorators"; -import {throwingComputedValueSetter} from "../api/computeddecorator"; import {hasInterceptors, IInterceptable, registerInterceptor, interceptChange} from "./intercept-utils"; import {IListenable, registerListener, hasListeners, notifyListeners} from "./listen-utils"; import {isSpyEnabled, spyReportStart, spyReportEnd} from "../core/spy"; @@ -70,10 +69,10 @@ export function setObservableObjectInstanceProperty(adm: ObservableObjectAdminis if (adm.values[propName]) adm.target[propName] = value; // the property setter will make 'value' reactive if needed. else - defineObservableProperty(adm, propName, value, true); + defineObservableProperty(adm, propName, value, true, undefined); } -export function defineObservableProperty(adm: ObservableObjectAdministration, propName: string, newValue, asInstanceProperty: boolean) { +export function defineObservableProperty(adm: ObservableObjectAdministration, propName: string, newValue, asInstanceProperty: boolean, setter) { if (asInstanceProperty) assertPropertyConfigurable(adm.target, propName); @@ -81,11 +80,20 @@ export function defineObservableProperty(adm: ObservableObjectAdministration, pr let name = `${adm.name}.${propName}`; let isComputed = true; - if (typeof newValue === "function" && newValue.length === 0 && !isAction(newValue)) - observable = new ComputedValue(newValue, adm.target, false, name); - else if (newValue instanceof AsStructure && typeof newValue.value === "function" && newValue.value.length === 0) - observable = new ComputedValue(newValue.value, adm.target, true, name); - else { + if (newValue instanceof ObservableValue) { + observable = newValue; + isComputed = false; + } else if (newValue instanceof ComputedValue) { + observable = newValue; + if (!newValue.scope) + newValue.scope = adm.target; + } else if (typeof newValue === "function" && newValue.length === 0 && !isAction(newValue)) { + // TODO: add warning in 2.6, see https://github.com/mobxjs/mobx/issues/421 + // TODO: remove in 3.0 + observable = new ComputedValue(newValue, adm.target, false, name, setter); + } else if (newValue instanceof AsStructure && typeof newValue.value === "function" && newValue.value.length === 0) { + observable = new ComputedValue(newValue.value, adm.target, true, name, setter); + } else { isComputed = false; if (hasInterceptors(adm)) { const change = interceptChange(adm, { @@ -139,11 +147,12 @@ export function generateComputedPropConfig(propName) { get: function() { return this.$mobx.values[propName].get(); }, - set: throwingComputedValueSetter + set: function(v) { + return this.$mobx.values[propName].set(v); + } }; } - export function setPropertyValue(instance, name: string, newValue) { const adm = instance.$mobx; const observable = adm.values[name]; diff --git a/test/babel/babel-tests.js b/test/babel/babel-tests.js index 2d34dce0a..e615732df 100644 --- a/test/babel/babel-tests.js +++ b/test/babel/babel-tests.js @@ -707,3 +707,22 @@ test("505, don't throw when accessing subclass fields in super constructor (babe t.deepEqual(values, { a: 1, b: 2}) // In the TS test b is undefined, which is actually the expected behavior? t.end() }) + +test('computed setter should succeed (babel)', function(t) { + class Bla { + @observable a = 3; + @computed get propX() { + return this.a * 2; + } + set propX(v) { + this.a = v + } + } + + const b = new Bla(); + t.equal(b.propX, 6); + b.propX = 4; + t.equal(b.propX, 8); + + t.end(); +}); diff --git a/test/observables.js b/test/observables.js index 553a36da4..63a4bdb3f 100644 --- a/test/observables.js +++ b/test/observables.js @@ -1,3 +1,5 @@ +"use strict" + var test = require('tape'); var mobx = require('..'); var m = mobx; @@ -1742,4 +1744,43 @@ test('atom events #427', t => { d() t.equal(stop, 2) t.end() -}) \ No newline at end of file +}) + +test("support computed property getters / setters", t => { + let a = observable({ + size: 1, + volume: mobx.computed(function() { + return this.size * this.size + }) + }) + + t.equal(a.volume, 1) + a.size = 3 + t.equal(a.volume, 9) + + t.throws(() => a.volume = 9, /It is not possible to assign a new value to a computed value/) + + a = {} + mobx.extendObservable(a, { + size: 2, + volume: mobx.computed( + function() { return this.size * this.size }, + function(v) { this.size = Math.sqrt(v) } + ) + }) + + const values = [] + const d = mobx.autorun(() => values.push(a.volume)) + + a.volume = 9 + mobx.transaction(() => { + a.volume = 100 + a.volume = 64 + }) + + t.deepEqual(values, [4, 9, 64]) + t.deepEqual(a.size, 8) + + d() + t.end() +}) diff --git a/test/typescript-tests.ts b/test/typescript-tests.ts index 998eefc42..82abbd2db 100644 --- a/test/typescript-tests.ts +++ b/test/typescript-tests.ts @@ -233,17 +233,22 @@ test('box', function(t) { t.end(); }) -test('observable setter should fail', function(t) { - t.throws(() => { - class Bla { - @computed get propX() { - return 3; - } - set propX(v) { - - } +test('computed setter should succeed', function(t) { + class Bla { + @observable a = 3; + @computed get propX() { + return this.a * 2; + } + set propX(v) { + this.a = v } - }, 'propX'); + } + + const b = new Bla(); + t.equal(b.propX, 6); + b.propX = 4; + t.equal(b.propX, 8); + t.end(); });