Mobx: Introduce support for Symbol named observable properties

[chengcyber]

Welcome to MobX. Please provide as much relevant information as possible!

I have a:

1. [x] Question: Feel free to just state your question. For a quick answer, there are usually people online at our Gitter channel
2. [ ] Documentation improvement. Please don't open an issue but create a PR instead!
3. [ ] Issue:
   
   
   
   • [ ] Provide error messages including stacktrace
   
   
   • [x] Provide a minimal sample reproduction. Create a reproduction based on this sandbox
   
   
   • [ ] Did you check this issue wasn't filed before?
   
   
   • [ ] Elaborate on your issue. What behavior did you expect?
   
   
   • [ ] State the versions of MobX and relevant libraries. Which browser / node / ... version?
   
   
4. [ ] Idea:
   
   
   
   • [ ] What problem would it solve for you?
   
   
   • [ ] Do you think others will benefit from this change as well and it should in core package (see also mobx-utils)?
   
   
   • [ ] Are you willing to (attempt) a PR yourself?
   
   

Please tick the appropriate boxes. Feel free to remove the _other_ sections.

Please be sure to close your issues promptly.

What

error 'set' on proxy: trap returned falsish for property 'Symbol(count)' threw when using Symbol as key in an observable,

How to reproduce

1. click the add button in normal section works fine
2. click the add button in symbol section broken :(

[chengcyber]

Addition, if using mobx@4, it just fails silently rather than throwing the error out in version 5.

[ItamarShDev]

I fail to understand the use-case of this scenario, you want to create a dynamic key?
isn't it what extendObservable does?
ref: https://mobx.js.org/refguide/extend-observable.html

[chengcyber]

Hi @ItamarShDev,

The question is mobx doesn't work when using a Symbol as an object key.

// works
observable({
  count: 0,
})

// broken
observable({
  Symbol('count'): 0,
})

I am maintaining a vue.js project, and bring mobx to do state management rather than the native vue way. The original practice of this project using sth like Symbol('isSelected') as an observable object key, and it works now, but fails in mobx. So I am curious about the difference caused by my misuse of mobx or mobx doesn't support it.

[ItamarShDev]

Thanks for the explanation @kimochg !
This is an interesting problem.

[jlgrall]

If Symbols where supported, could it conflict with current or future well-known symbols like: Symbol.iterator, Symbol.toStringTag, etc. ?

Should there be a list of excluded symbols ?

Disclaimer: I am currently taking advantage of the fact that symbols are ignored by MobX in my MobX extension library: https://github.com/jlgrall/mobx-patch-computedTree, but I can find another way.

[urugator]

Mobx ignores non-enumerable props (like symbols) in observable/extendObservable.
The symbol is firstly introduced to the observable object during the assignment (in incSymbol).
Mobx4 doesn't support dynamic keys, so the standard JS behavior (new non-observable property is added to object) takes place, therefore no error.
In Mobx5 proxy handler returns false for non-string keys, resulting in TypeError.

If Symbols where supported, could it conflict with current or future well-known symbols like: Symbol.iterator, Symbol.toStringTag, etc. ?

I don't think so. Symbols are accessible via Object.getOwnPropertySymbols(), returning an empty array on empty objects. The argument passed to observable/extendObservable represents simple property list - not an arbitrary "smart" object (eg an object implementing a well known symbol).
So if there is a symbol, it's introduced by user and should be made observable imho (no matter what symbol it is, similary to providing toString or valueOf).

[ItamarShDev]

@mweststrate What do you think about this issue?

[dr0p]

:+1: from me also. We I'm trying to use symbols as keys for mixin attached private properties to a class and also make them observable, but mobx throws (up) :smile:

[mweststrate]

I think symbol named properties should probably behave the same as string named properties. That is, if they are enumerable, they should be picked up. Marking it as feature.

Contributions are welcome. Even a solid test suite would already give a great head start! Note that this has quite some impact on typings as well, as in things like extendObservable, keys and entries can have symbols in all positions where property names are returned now.

I guess ObservableMap could be left out of the equation for now?

[xuchaoqian]

@mweststrate Looking forward to this feature. I am trying the same thing with @dr0p , but mobx doesn't work.

Just as the following code snippet:

`function decorateProp(params = {}) {
  params = parseParams(params);
  return function (target, name, descriptor) {
    const privateName = Symbol(name); // `_${name}`;
    params.observable(target, privateName, descriptor);
    return computed(target, name, buildDescriptor(
        params, privateName
    ));
  };
}`

But mobx throws this error:

`mobx.module.js:98 Uncaught Error: [mobx] invalid option for (extend)observable: configurable
    at invariant$$1 (mobx.module.js:98)
    at fail$$1 (mobx.module.js:93)
    at assertValidOption (mobx.module.js:424)
    at Array.forEach (<anonymous>)
    at asCreateObservableOptions$$1 (mobx.module.js:434)
    at Function.object (mobx.module.js:498)
    at Object.createObservable [as observable] (mobx.module.js:463)
    at prop.js:49
    at prop (prop.js:60)
    at web.js:182433`

[jheeffer]

I'd like to make my own iterable an observable object, but this issue is preventing me from doing so : ). In this case though, the Symbol shouldn't be enumerable, preferably.

Basic use case for us is an iterable datastructure whose data source can differ (e.g. array or a generator function).
https://jsfiddle.net/jasperh/x7mz4k8w/8/

[urugator]

@jheeffer If I follow correctly, you don't need the symbol prop observable. So if you can live with some kind of factory function, you can introduce the iterator to observable object:

const bar = mobx.observable(foo)
Object.defineProperty(bar, Symbol.iterator, {
  enumerable: false,
  get() {
    return this.x[Symbol.iterator].bind(this.x); 
  }
})

[jheeffer]

@urugator you're right, that could work too. Out of the box keeping an object iterable would be nice but I understand the trade-offs : )

[loklaan]

👋 Hey folks, I've tried to kicked this off in PR #1944, please feel free to have a look n' review if you have the time.

[mweststrate]

Released as 4.10.0 / 5.10.0. Thanks @loklaan !

[loklaan]

YES 🎉