Tracked mirror approach for prop notifications

[betocantu93]

@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 addObserver and removeObserver to 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.mirror property. 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.

get myProp() {
  return this.args.changeset.get(this.args.path);
}

@computed('myProp')
get someComputed() {
 return ....
}

//or
@computed(`args.changeset.mirror.yourpath`)
...

Here's the sandbox im using for this DEMO:
https://github.com/betocantu93/changeset-tests/tree/try-mirror

Notice: please use try-mirror branch

[snewcomer]

I am liking the ideas we are exploring here!

[snewcomer]

I think we would want both? Or is the tracked-built-ins intended for things like @tracked _changes; as well?

[betocantu93]

🤔 not sure, TBH! Gonna have a look

[snewcomer]

So is changest.addObserver a common pattern that we should support and/or encourage?

[betocantu93]

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

[snewcomer]

Does this handle the obj.key case as well? Or do we have the ability to handle this case?

[betocantu93]

I think yes? Because of the Proxy traps all access 🤔

[snewcomer]

Is this meant to be accessed anywhere? I don't see this.mirror anywhere.

[betocantu93]

Mainly for public access, for the classic computeds case

[snewcomer]

Is this meant to be safeGet(this.mirror, key)?

[betocantu93]

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(){}

[snewcomer]

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.

[snewcomer]

Or am I missing a crucial need for notifyPropertyChange, even in the new reactivity world?

[betocantu93]

🤔🤔🤔 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 [email protected]