Tracked mirror approach for prop notifications #591
Conversation
…to spike-fix-prop-notification
…to spike-fix-prop-notification
betocantu93
changed the title
| import ArrayProxy from '@ember/array/proxy'; | ||
| import ObjectProxy from '@ember/object/proxy'; | ||
| import { notifyPropertyChange } from '@ember/object'; | ||
| import mergeDeep from './utils/merge-deep'; | ||
| import isObject from './utils/is-object'; | ||
| import { tracked } from '@glimmer/tracking'; | ||
| import { tracked } from 'tracked-built-ins'; |
There was a problem hiding this comment.
I think we would want both? Or is the tracked-built-ins intended for things like @tracked _changes; as well?
There was a problem hiding this comment.
🤔 not sure, TBH! Gonna have a look
| } | ||
|
|
||
| deepAddObserver(this[VIRTUAL_MIRROR], key, callback); | ||
| } |
There was a problem hiding this comment.
So is changest.addObserver a common pattern that we should support and/or encourage?
There was a problem hiding this comment.
Manually adding observers in general is not that common, I needed them for a very specific use case that I'm not entirely sure if it could be solved by native getters and this mirror approach, still having a way to add them gives more flexibility and an actual way to do it, which doesn't exist today, also... adding them to the content have it's own set of inconveniences
| get(key) { | ||
| safeGet(this[VIRTUAL_MIRROR], key); //consume tag for native tracked dependencies | ||
| return super.get(...arguments); | ||
| } |
There was a problem hiding this comment.
Does this handle the obj.key case as well? Or do we have the ability to handle this case?
There was a problem hiding this comment.
I think yes? Because of the Proxy traps all access 🤔
| @dependentKeyCompat | ||
| get mirror() { | ||
| return this[VIRTUAL_MIRROR]; | ||
| } |
There was a problem hiding this comment.
Is this meant to be accessed anywhere? I don't see this.mirror anywhere.
There was a problem hiding this comment.
Mainly for public access, for the classic computeds case
| @@ -188,6 +280,26 @@ export class EmberChangeset extends BufferedChangeset { | |||
|
|
|||
| return this; | |||
| } | |||
|
|
|||
| get(key) { | |||
| safeGet(this[VIRTUAL_MIRROR], key); //consume tag for native tracked dependencies | |||
There was a problem hiding this comment.
Is this meant to be safeGet(this.mirror, key)?
There was a problem hiding this comment.
I followed the pattern of using this[KEYWORD] on internal usages, the get mirror(){} is meant to be used by client code, public access, like.... get data(){}
| } | ||
| } | ||
| const pathToNotify = maybeDynamicPathToNotify ? maybeDynamicPathToNotify : lastPath; | ||
| notifyPropertyChange(current, pathToNotify); |
There was a problem hiding this comment.
I wonder if we should make notifyPropertyChange opt-in in a next major 4.0 release? Generally, we gave patterns that obfuscate the need for these reactivity systems.
There was a problem hiding this comment.
Or am I missing a crucial need for notifyPropertyChange, even in the new reactivity world?
There was a problem hiding this comment.
🤔🤔🤔 maybe it's no longer needed, you're right, gonna have a look, one thing I still need to look for is reactivity around arrays, i.e. computeds which use changeset.someArray.@each.value
@snewcomer This is another approach for #586, property notifications
We use tracked-built-ins to have a common tracked object reference that we can keep notifying of changes by lazily consuming tags on every changeset.get.
This PR also implements
addObserverandremoveObserverto allow client code to add / remove observers in an ergonomic way to a changeset.The only "downside" AFAIK is that for classic computeds, we have to manually subscribe to the
changeset.mirrorproperty. One way to solve this, is to just use native getters or, create a native getter which consumes the value and then add that native getter to the dependencies of your computed, i.e.Here's the sandbox im using for this DEMO:
https://github.com/betocantu93/changeset-tests/tree/try-mirror
Notice: please use try-mirror branch