Object.defineProperty and useProxies: "ifavailable"

[BATCOH]

Intended outcome:
A way to define a property on an observable object with useProxies: "ifavailable" and without error.

Actual outcome:
Mobx with useProxies: "ifavailable" gives an error in dev mode when trying to set an observable object property via Object.defineProperty.

Obviously in this case the error is relevant, the problem is with its description.

Use 'defineProperty' from 'mobx' instead.

But there is no defineProperty export in mobx. I think at least the description should be corrected or such method should be provided.
And I haven't found an easy way to suppress this error if I'm sure of what I'm doing other than disabling useProxies: 'ifavailable'.

Specifically, mobx-state-tree uses Object.defineProperty to store some internal data. And working with mobx-state-tree in "ifavailable" mode is currently impossible.
https://codesandbox.io/s/mobx-state-tree-todolist-ifavailable-t9w2z?file=/index.js
Maybe I should post an issue to MST too, but it doesn't seem to make much sense without offering a solution to replace Object.defineProperty with something else.

How to reproduce the issue:
https://codesandbox.io/s/mobx-defineproperty-16wyc?file=/src/index.js

Versions
"mobx": "^6.1.8"

[urugator]

Dangerous way of doing that: Object.defineProperty(obj[mobx.$mobx].target_, key, descriptor) (_getAdministration(obj).target_ also possible, but teeny weeny slower)
It will NOT:

• clean-up internal data associated with property (if any)
• invoke interceptors, spies, observers and reactions

It should be completely fine to use for storing hidden non-observable meta data and it should perform a lot better in such case.

If someone could pinpoint me to the MST sources from where the defineProperty call originates, I may provide more qualified advice or perhaps even PR.

One way or another, I will add the "safe" function to the api.

[mweststrate]

@urugator I'm wondering, should the current interception not just forward the defineProperty to the target using Reflect, rather than going through MobX administration? I don't think we officially support using defineProperty, and I think it should be just an at-own-risk meta programming tool (that maybe warn's if it is used on existing properties)

mobx/packages/mobx/src/types/dynamicobject.ts

Line 56 in 28f8a11

if (__DEV__) {

. Was there a specific reason why we rewire it to ObjectAdministration here?

[urugator]

I don't think we officially support

Support is mentioned in 6.1.0 changelog

Was there a specific reason

It's easier to support, than treat it as a special case. We still need to be able to define property internally. All the APIs that introduce new prop, eventually delegate to single method adm.defineProperty_ that handles property definition safely and uniformly. It's one less thing to worry about, we don't have to explain it's not supported and why - not just to users, but maintainers as well - it's really hard to keep track of all these special cases and reasonings, so better keep them at minimum.

Another more or less related thing is that these proxy traps not always map 1-to-1 to single method, they can be called by engine on different occasions. Eg. currently we don't support subscription for getOwnPropertyDescriptor, but we can't even throw an error from proxy handle, because it would throw during bunch of other operations, that we definitely want to support.
Now consider the policy of "ifavailable" - it throws if operation isn't supported - you cannot call these even as at-own-risk meta programming. Something that was maybe at-own-risk with proxies: "never", becomes completely forbidden on proxies: "ifavailable". So if we want be somehow consistent and keep warning users, then every use case supported by proxies, should have it's counter part in object-api. Which finally brings me to question:
Do you agree with adding ownKeys, getOwnPropertyNames, getOwnPropertySymbols to object-api? All of these are subject of ownKeys proxy trap. If not, we should change the error into something like "If you called Object.keys use keys from mobx, all other operations are forbidden in ES5 compatible mode.". Again, to me, it seems easier to just add these functions, rather than have to keep explaining...

[urugator]

Or we may actually want to rethink "ifavailable", because practically it's "never" with convinient warnings on devel.
Afaik "ifavailable" has no benefits on production - it's more likely the opposite - bit slower with possibly unexpected errors on ES5. EDIT ok maybe arrays are actually better perf wise? dunno
So maybe proxies: "devel" ("always" for devel, "never" for production), would make more sense. Or even better it should use proxies only for warning, but not for impl.
EDIT: never mind, I don't want to complicate it further at this point

[BATCOH]

If someone could pinpoint me to the MST sources from where the defineProperty call originates, I may provide more qualified advice or perhaps even PR.

@urugator
There are addHiddenFinalProp/addHiddenWritableProp utils in MST that uses defineProperty.
They are used to define hidden $treenode and toJSON on nodes, to initialize hooks on arrays/maps, and to add action invokers to model.

Also, defineProperty with addHiddenFinalProp/addHiddenWritableProp used to define computeds during instantiateViews on model.

It looks like that's all.

[urugator]

@BATCOH Since these worked prior 6.1 and Object.defineProperty(obj[mobx.$mobx].target_, key, descriptor) is analogous to pre-6.1 behavior, it should be safe to use this workaround. However, here
https://github.com/mobxjs/mobx-state-tree/blob/8ce9992ff5480e1e7c66803b6692354a1121d801/packages/mobx-state-tree/src/types/complex-types/model.ts#L495-L510
is an opportunity to make use of the new behavior. Redefining computed via defineProperty should be safe now.
(computed is by default non-configurable on non-plain objects, but since this object is proxied, it must be plain, therefore configurable) EDIT: I realized that I actually don't know whether this is plain, because I don't know whether it also throws here

[mweststrate]

Afaik "ifavailable" has no benefits on production - it's more likely the opposite - bit slower with possibly

Proxies used to be faster, is this no longer the case?

Object.defineProperty(obj[mobx.$mobx].target_, key, descriptor)

I think the work around in general is fine, MST has more specific integrations involving the $mobx symbol. But .target_ will be mangled so this might not work in production builds.

Possible mitigation:

1. Have the trap for defineProperty throw in ifavailable without offering an alternative
2. Expose an unofficial _defineProperty for MST which works as proposed above (if target is a proxy)?

[urugator]

Proxies used to be faster

I've not tested, but I can't imagine how they could be faster (with object). There is exactly the same machinery (you still have a target_ object with the same getters etc) + extra proxy. It's most likely possible to have a better seperate dynamic object impl, but currently we don't have it. Still I don't think there would be a significant difference between trap and getters/setter perf - maybe for initialization. And it's not just about get/set right ... eg Object.entries, which would probably just call some optimized native code, must suddenly get a list of keys through ownKeys and call getOwnPropertyDescriptor trap for each key to filter them by enumerability + get to retrieve a value. However, I've already edited the comment and I don't want to push this any further, let's leave it as it is.

Possible mitigation:

Exposing and using the official defineProperty from object-api, that works just like Object.defineProperty + proxy is not an option?

[urugator]

Once released, MST should call defineProperty from mobx.