[BUGFIX beta] Cleanup CP set, volatile and various related things. #11983
Conversation
Breakup set method into a top level branching method that has 4 paths, readonly check/throw, clobber set, volatile set or normal set. `volatile()` does not simply mean don’t cache, it means this value is inherently not observable and/or you need to manually notify/cache the value. ArrayProxy is a good example, length is observable but has to be manually notified for NativeArray mixin, so ArrayProxy just delegates to the underlying array but doesn’t setup DKs or any notification since it is notified already by the mutation methods. It is not an escape value for you not getting the correct DKs or it seems to solve some bug I can’t figure out. It is not the equivalent of the old sprout core behavior and hasn’t been for a while. If you have a dependency that does not fit into a DK, you can notifyPropertyChange() manually. If you don’t understand the above you should not be using `volatile` it has an extremely narrow use case.
|
@locks the above words may make nice doc additions to volatile |
|
|
||
| // don't create objects just to invalidate | ||
| let meta = obj.__ember_meta__; | ||
| if (meta.source !== obj) { |
There was a problem hiding this comment.
i think we need to guard against no meta exists (or no longer existing) right here.
|
#11983 (comment) cc @emberjs/guides-managers |
|
|
||
| this._suspended = obj; | ||
| if (!this._setter) { | ||
| return this.clobberSet(obj, keyName, value); |
There was a problem hiding this comment.
I remember that at some point in the past we considered to warn/deprecate in this case, unless you mark the CP like overridable, because it's usually not what you intend to to.
Just checking if it's a good moment to do that.
There was a problem hiding this comment.
@cibernox - There was an RFC for that (and you have an open PR doing this IIRC). Sadly, it is too late in the beta cycle to add now.
|
Updated to address remaining feedback/tweaks. |
[BUGFIX beta] Cleanup CP set, volatile and various related things.
|
@krisselden / @stefanpenner - Could one of y'all cherry pick this into beta (or submit a PR against the beta branch directly)? I tried to cherry-pick, but due to the meta refactor on master (not in beta) I got to a point of total confusion and don't want to mess this stuff up in the beta branch... |
|
If you can remind me tomorrow afternoon I will absolutely! |
|
@stefanpenner - It's afternoon here... 😺 |
Breakup set method into a top level branching method that has 4 paths, readonly check/throw, clobber set, volatile set or normal set.
volatile()does not simply mean don’t cache, it means this value is inherently not observable and/or you need to manually notify/cache the value.ArrayProxy is a good example, length is observable but has to be manually notified for NativeArray mixin, so ArrayProxy just delegates to the underlying array but doesn’t setup DKs or any notification since it is notified already by the mutation methods.
It is not an escape value for you not getting the correct DKs or it seems to solve some bug I can’t figure out. It is not the equivalent of the old sprout core behavior and hasn’t been for a while. If you have a dependency that does not fit into a DK, you can notifyPropertyChange() manually.
If you don’t understand the above you should not be using
volatileit has an extremely narrow use case.