-
-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added possibility to respect observable decorator variants, sets can only be observed shallowly #313
Conversation
…only be observed shallowly mobxjs#298, mobxjs#312
This is a draft because tests are missing |
} | ||
|
||
function getKeyAnnotationType(thing: any, key: string): string | undefined { | ||
return _getAdministration(thing)?.appliedAnnotations_?.[key]?.annotationType_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appliedAnnotations_
is private, mangled and actually not even available in production build:
https://github.com/mobxjs/mobx/blob/78d1aa2362b4dc5d521518688d6ac7e2d4f7ad3a/packages/mobx/src/types/observableobject.ts#L114-L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if there is a way to get to the annotations in production build? Or is this idea just not possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that you've misunderstood how ref
and shallow
works. They say how a non-observable value (plain object/array/set/map) should be converted into observable - if the value is already observable, these decorators have no effect. So if you have an observable Entity
it remains observable even if you assign it to ref prop or push to shallow array. Since these don't remove observability, it would be weird if deepObserve
would ignore it.
You could additionally wrap the Entity
into something actually non-observable to stop the recursion:
@observable.ref _entity = { ref: entity }
get entity() {
return this._entity.ref;
}
set entity(entity) {
this._entity = { ref: entity }
}
get to the annotations in production build
I think you can get the enhancer
via getAtom(thing, prop?).enhancer
, but to find out what kind of enhancer it is, you actually have to call it and check the result, like this:
#170 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's exactly what we want. We have entities which reference each other (decorated with observable.ref
) and each one have their own observable properties. We want to observe the entities in a way that we get info only when their own property changes, not property of some other entity then reference.
class EntityA {
@observable.ref entityB?: EntityB
@observable propertyA: any
@observable somethingDeep: string[] = []
constructor() {
makeObservable(this)
}
}
class EntityB {
@observable propertyB: string
constructor() {
makeObservable(this)
}
}
const entityA = new EntityA()
const entityB = new EntityB()
entityA.entityB = entityB
const dispose = deepObserve(entityA, console.log)
entityA.propertyA = 'something' // I want this to be logged
entityA.somethingDeep.push('something') // I want this to be logged
entityB.propertyB = 'something' // I don't want this to be logged
dispose()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you should use observe
, not deepObserve
. As @urugator noted ref/shallow modifiers are completely unrelated to deep resp shallow observing. Simply put, deepObserve
is a wrapper around observe
, that sets ups new observe
listeners recursively, until no observables could be reached anymore. So if an an observable.ref points to an observable, it will recurse into it. MobX-state-tree has a semantic distinction between 'owning' an object, and loosely referring to it, but in MobX we make no such distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But with observe
I wouldn't get the update when doing entityA.somethingDeep.push('something')
. Anyway it looks like it is not possible to do this in the production build or you just don't want this to be included.
Closing PR
Kind of fixes #298 (at least they can be observed shallowly) and fixes #312