Looking at current decorators internal implementation - it eagerly creates new ComputedValue for every decorated property for every new instance
function decorate_20223_(this: Annotation, get, context: ClassGetterDecoratorContext) {
if (__DEV__) {
assert20223DecoratorType(context, ["getter"])
}
const ann = this
const { name: key, addInitializer } = context
addInitializer(function () {
const adm: ObservableObjectAdministration = asObservableObject(this)[$mobx]
const options = {
...ann.options_,
get,
context: this
}
options.name ||= __DEV__
? `${adm.name_}.${key.toString()}`
: `ObservableObject.${key.toString()}`
adm.values_.set(key, new ComputedValue(options))
})
return function () {
return this[$mobx].getObservablePropValue_(key)
}
}
I think it is quite a waste of memory and performance:
- I think it is safe to say, many of computed properties of some instances will never ever be used
- say we have 1k instances, each have 5 @computed getters
- say on each instance, we only ever use 2 out of those 5 during their lifetime
- as a result - 3000
ComputedValue instances were created, while never being used
- those 3000 took some time to create and did occupy some memory before being disposed
What, if decorator would be implemented something like in example below.
Here, we do not add addInitialize and create computed value once on the first getter use
- if computed value is never used -
ComputedValue is never created
- as there is no
addInitialize - it should have lower performance penantly eg when creating 1000s of new instances with decorators
- AFAIK the only initial runtime cost will be similar internal prototype update during class declaration. Creating class instance will have comparable performance to creating normal, undecorated instance
import { ComputedValue } from "mobx/dist/internal";
const COMPUTED_SYMBOL = Symbol("computed");
type Key = string | symbol;
type ComputedMap = Record<Key, ComputedValue<any>>;
function initializeComputed(instance: any, key: Key, getter: () => any): ComputedValue<any> {
// if needed - initialize our map of computed values
const computedMap: ComputedMap = (instance[COMPUTED_SYMBOL] ??= {});
// if needed - initialize new `ComputedValue`
return (computedMap[key] ??= new ComputedValue({
get: getter,
context: instance,
}));
}
export function $computed(get: () => any, context: ClassGetterDecoratorContext) {
const { name: key } = context;
return function (this: any) {
// when used - initialize computed if needed and get its value
return initializeComputed(this, key, get).get();
};
}
Intended outcome:
When creating instance with @computed getter, but never using it - ComputedValue should never be created
Actual outcome:
ComputedValue is created even if we never use it
How to reproduce the issue:
On created instance, check $mobx special property - computed value is there.
Versions
6.15.0
Looking at current decorators internal implementation - it eagerly creates new
ComputedValuefor every decorated property for every new instanceI think it is quite a waste of memory and performance:
ComputedValueinstances were created, while never being usedWhat, if decorator would be implemented something like in example below.
Here, we do not add
addInitializeand create computed value once on the first getter useComputedValueis never createdaddInitialize- it should have lower performance penantly eg when creating 1000s of new instances with decoratorsIntended outcome:
When creating instance with
@computedgetter, but never using it -ComputedValueshould never be createdActual outcome:
ComputedValueis created even if we never use itHow to reproduce the issue:
On created instance, check
$mobxspecial property - computed value is there.Versions
6.15.0