Mobx: Decorators make properties readonly if their key is a symbol computed property in TS (phew)

[StephenHaney]

Ok, that's a mouthful. Here's the situation in question:

const someSymbol = Symbol('whatever');

class MyClass {
  @observable
  [someSymbol] = true;

  @action.bound
  changeSymbolProperty() {
    this[someSymbol] = false;
  }
}

const class = new Class();

class.changeSymbolProperty()
// This triggers an error: TypeError: Cannot assign to read only property [someSymbol]

Details:

1. MobX version 5.14.0
2. TS version 3.6.4
3. From my understanding, TypeScript's decorators implementation does not provide any default meta data. So if a decorator doesn't specify writable, it falls back to the default writable = false;
4. For some reason this isn't an issue with normal property keys, or computed property keys that use strings. It only happens with symbols.
5. You have to use TypeScript to compile the example to get this error. If you use Babel, you get a syntax error (unexpected token on the equals sign in [someSymbol] = true – a bug in babel with decorators on computed property keys, unrelated to this problem.

Since it doesn't make sense that you'd want to observe something that's readonly, can we add an explicit writable = true to the observable decorator?

Is MobX even responsible for caring about how TypeScript implements decorators? I guess that's the downside of "experimental syntax".

---

So, the fallback is to use decorate on these symbol computed property keys, which works fine. Most of the time.

decorate gets confused if you're creating an abstract class. It doesn't matter if the property is a string, symbol, computed, or not. This might be a separate issue, but here's an example:

abstract class MyClass {
  abstract someAbstractProperty = 'whatever';

  localProperty = 'some value';
}

decorate(MyClass, {
  localProperty: observable
});
// Error: Object literal may only specify known properties, and 'localProperty' does not exist in type '{ prototype?: MethodDecorator | PropertyDecorator | MethodDecorator[] | PropertyDecorator[] | undefined; }'

I'm guessing abstract class messes up that prototype typing.

---

What's it all mean? Two things:

First, @observable turns properties readonly in this situation:

1. Computed property key
2. Key is a symbol
3. When using TypeScript's decorator implementation (babel's fails too but it's a syntax error bug in babel)

Two, I don't think there's a way to make this situation observable:

1. Computed property key (string or symbol)
2. In an Abstract class

[mweststrate]

Especially the first one is interesting, you might want to check if the code somewhere makes an assumption that the key is a string, where it shouldn't. (e.g. a typeof or something). Since it is a runtime error, it might very well be an error on MobX's site.

For the second case, I think the TS error is actually correct, as properties only live on instances, and since the class is abstract, it will never have such a property. Which doesn't help in this case of course. I'd personally worry not to much about that case and throw a // ts-ignore in if it behaves further correctly (it should, observable properties can be defined on prototypes).

[urugator]

Maybe?

https://github.com/mobxjs/mobx/blob/2b6f301a743f7ad259952f2130883969ad0a43d2/src/api/observable.ts#L80-L84
=>

if (typeof arguments[1] === "string" || typeof arguments[1] === "symbol") {

[StephenHaney]

~Hmm hmm, tried changing that, still getting the cannot assign to read only property error. I also tried putting implicit writable: true in all possible descriptors that mobx creates, without any luck.~

Cancel the above, more testing in progress.

I can use other library's decorators on the same property without it changing the property to readonly, so it seems specific to something that @observable is doing.

I'll keep trying things out, just wanted to report back this additional info.

[urugator]

Not sure if related, but this could need to adjust as well:
https://github.com/mobxjs/mobx/blob/6c1d807a44dfc32567f3e55231eb1464a2155826/src/utils/decorators.ts#L114-L118

[StephenHaney]

Aah yes, that seems to have at least changed the situation ;)

After updating those two places to check for typeof symbol as well as typeof string, I get an infinite loop here:

function createPropertyInitializerDescriptor(prop, enumerable) {
    var cache = enumerable ? enumerableDescriptorCache : nonEnumerableDescriptorCache;
    return (cache[prop] ||
        (cache[prop] = {
            configurable: true,
            enumerable: enumerable,
            get: function () {
                console.log('reading start: ' + String(prop));
                initializeInstance(this);
                return this[prop];
            },
            set: function (value) {
                // NOTICE these console logs I added
                console.log('starting with: ' + String(prop));
                initializeInstance(this);
                this[prop] = value;
            }
        }));

[urugator]

And now it occured to me that it can be number as well ... actually it seems that whatever type you use as computed key, it's passed to decorator as second arg ... however only number/string/symbol make sense to take into account for the prupose of distinguishing decorator call from normal function call (I suppose?)... I've never been a fan of this overloading tbh...

[mweststrate]

Probably offtopic, but numbers are not really a case, as they are coerced to strings:

> const x = {}
undefined
> Object.defineProperty(x, "3", {value: 3 }){}
> Object.defineProperty(x, 3, {value: 4 })
Thrown:TypeError: Cannot redefine property: 3    
at Function.defineProperty (<anonymous>)

@StephenHaney would you be interested in creating a PR with a _minimal_ unit test (and the partial fix?), that would it make a lot easier to look into it and see if can be easily addressed :)

[StephenHaney]

Yes, definitely. I'll spend a bit more time debugging and then put something together.

[urugator]

@mweststrate Yes, but they're not coerced at the time they reach the decorator, consider:

function myDec(target: any, propertyKey: string, descriptor: PropertyDescriptor) {
  console.log(typeof propertyKey); // object/number/symbol/whatever
}

const objectKey = {}; // or number or whatever
class MyClass { 
  @myDec
  [objectKey] = "val";  
}

So we are unable to distinquish decorator call from normal call based on the second arg's type...

EDIT: but yea the workaround is trivial in such case, so probably nothing to worry about

[urugator]

Unrelated, but these :
https://github.com/mobxjs/mobx/blob/6c1d807a44dfc32567f3e55231eb1464a2155826/src/utils/decorators.ts#L29-L30
probably needs to be changed to

const enumerableDescriptorCache: { [prop: string]: PropertyDescriptor } = Object.create();
const nonEnumerableDescriptorCache: { [prop: string]: PropertyDescriptor } = Object.create();

otherwise the cache is preoccupied by default inherited props (toString etc)...

[urugator]

This seems suspicious
https://github.com/mobxjs/mobx/blob/6c1d807a44dfc32567f3e55231eb1464a2155826/src/utils/decorators.ts#L60-L63
as for-in skips symbols and non-enumerables

[StephenHaney]

Created the PR here https://github.com/mobxjs/mobx/pull/2175 with a straightforward test case that does reproduce the same issues. I'll continue playing around looking for the fix as well.

[StephenHaney]

This seems suspicious

Interesting! Are symbols used for internal mobx logic and being purposefully avoided? I changed it to loop through all keys and got new crashes, which makes me wonder if that code was purposefully avoiding symbols.

[urugator]

Afaik not, they just weren't around (or considered) at the time it was written and these loops were optimized for performance (targeting ES5 env).
You may try to send screen/stack if you get stuck on something...

[StephenHaney]

I'm actually at a bit of a loss – @urugator you are correct that the loop in question is dropping off symbols. I'm trying to correct it by doing something like this:

        const keys = [
            ...Object.getOwnPropertySymbols(decorators),
            ...Object.keys(decorators)
        ];
        for (const key of keys) {
            const d = decorators[key]; // Error: Type 'symbol' cannot be used as an index type
            d.propertyCreator(target, d.prop, d.descriptor, d.decoratorTarget, d.decoratorArguments)
        }

However, TypeScript does not allow you to use symbols to look up values in this way... and I can't find a way to do it without having the literal reference to the original unique token that was used to create the property.

If there's not a way to iterate through object properties that are symbols in TS, perhaps symbol keys shouldn't be supported. Maybe I'm missing a way to do this. I'll revisit a bit later.

[urugator]

I am not a TS user, but from I could find, it's basically a 4 years old issue...
Can't you "trick" the type system with something like key as unknown as string or disable it somehow // @ts-ignore?

[StephenHaney]

Yes, casting it as any allowed it to build and now all tests are passing (including the new one I added for symbol property keys).

That's all here:
https://github.com/mobxjs/mobx/pull/2175

If we all think this should be a supported use case and that this is the appropriate fix, I can finish the PR by updating change-logs and such. What do you all think?

[urugator]

I think go ahead :)

[mweststrate]

Published as 5.15.0

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.