spike for prop notification

[betocantu93]

Hello, this PR seems to fix a few bugs. (not sure if its the right way).

• 517 #585 #499 #437

It seems that notifyPropertyChange doesn't work for nested paths, so this PR runs notifyPropertyChange on deepest defined ancestor also ensures to always set, so it can return to initial value, which is the bug described in #585 but making sure it cleans the changes if the oldValue is equal to the newValue

[snewcomer]

I see where you are going with this and it seems reasonable! Do you have any unanswered questions or blockers?

[betocantu93]

One question: it's ok the tryContent approach? which looks for the obj to notify, via hardcoded .content and ._content paths, basically notifying changests and proxy alike objects?

[betocantu93]

@snewcomer Dug a little bit more on this today to try to fix the tests and found out a few things...

1. addObserver works for deep keys, i.e: you can do object.addObserver('foo.bar.baz') and it would work, meaning that if any of these nested objects gets notified by ember, the callback would run.
2. notifyPropertyChange doesn't work for deep keys, and thus this PR introduces deepNotifyPropertyChange i.e you can't do this: notifyPropertyChange(object, 'foo.bar.baz') it won't work. For this to work you would need to do notifyPropertyChange(object.foo.bar, 'baz')
3. Adding observers directly to changeset changeset.addObserver doesn't work as expected for a couple of reasons:

We traverse through proxies to find the nearest defined parent to notify at that level. but each of these are proxies (native proxies?) which are different objects from the ones that the clients 'subscribed' via adding the observer, and thus will never get notified, that's why notifying to the _content or content is important

At least with the current design, adding observers to the changeset should be added to data or _content so you can reliably add observers deeply and get notified, because the deepNotifyPropertyChange will always try to notify to the _content or content of the proxies.

One downside of this is that we won't get the changeset as this inside the callback, but unlock adding them even with addObserver directly, addObserver(changeset.data, 'deep.nested.key', () => {})

Overall with this PR we get deep observers and specific prop notifications, playing well with most octane patterns and also classic.

We could... to allow a more ergonomic api, – but I think it might do more harm than good – could potentially do this inside the proxy getter:

export function Changeset(obj, validateFn = defaultValidatorFn, validationMap = {}, options = {}) {
  const c = changeset(obj, validateFn, validationMap, options);

  return new Proxy(c, {
    get(targetBuffer, key, receiver) {

      if(key.toString() === 'addObserver') {
        return targetBuffer._content?.addObserver?.bind(targetBuffer._content)
      }

      if(key.toString() === 'removeObserver') {
        return targetBuffer._content?.removeObserver?.bind(targetBuffer._content)
      }

      const res = targetBuffer.get(key.toString());

      return res;
    },

    set(targetBuffer, key, value /*, receiver*/) {
      targetBuffer.set(key.toString(), value);
      return true;
    },
  });
}

This would make that changeset.addObserver('my.deep.keys', () => {}) just 'work'

but

using addObserver(changeset, 'my.deep.keys', () => {}); wouldn't work because of the deepNotifyPropertyChange notifying different objects, and so I think teaching about changeset.data.addObserver might be a better idea.

One problem with this approach

With this approach if the changeset.data changes from outside, you will get notified. Not entirely sure how to avoid this or if this could all be solved differently.

[betocantu93]

I'm using this with ember-m3, MegamorphicModels and seems to work great too

[betocantu93]

OK, so dug a lot more today, ObjectTreeNode proxies return different references between proxy access if the prop doesn't exists inside node.content here: https://github.com/validated-changeset/validated-changeset/blob/d9e54bea45d0ea88d4de7e9faf39733c050c755b/src/utils/object-tree-node.ts#L39

And so we can't realiaby notify, and so I propose:

We'll try to find the deepest ancestor which could be notified i.e the reference to the underlaying content is the same on each access, which means that for deeply "fully" dynamic paths, we can't go deep or specific at all as I wished.

For now, for my bullet proof observer use case, I think I incline to use a more event oriented api and probably could be documented as a way to observe reliably about changes i.e. changeset.on('afterValidation', this.myOtherwiseObserver);

In the best scenario, if the content isn't dynamic at all (the content has all the structure in advance), it should be specific with the notifications.

In the worst scenario, the content is fully dynamic, we will notify from the top.

In the middle ground scenario, we traverse until we're out of CONTENT defined paths and notify there.

I recognize this solution probably is not the best one, but could be a known limitation, unless some other approach comes by.

Another "solution" could be to have some sort of @tracked _mirror from tracked-built-ins which just mirrors the content plus the changes structure, we don't actually care about the actual values, just about a common reference which we could be notifying by setting, and we could just addobservers or use getters around that reference, but not sure if it makes sense.

[betocantu93]

Had to bump node because one dep needs >=12 now? it prompted because of me using volta, which was pin to node@10~

error ansi-regex@6.0.0: The engine "node" is incompatible with this module. Expected version ">=12". Got "10.22.0"
error Found incompatible module.

[betocantu93]

I noticed assigning curr=path[i] was triggering set on the proxy, causing trouble to fix #585 But I think we can just traverse "content", or hasOwnProperty on that content as suggested, and that does fixs #585...

But in my sandbox:

https://github.com/betocantu93/changeset-tests

Stumbled with another bug, when you try to changeset.get('super.deep.key') where content doesn't have such structure, it fails on this line because we are trying to getDeep on undefined.

https://github.com/validated-changeset/validated-changeset/blob/d9e54bea45d0ea88d4de7e9faf39733c050c755b/src/index.ts#L963

Not sure how that should be fixed over there, any suggestion?

[snewcomer]

@betocantu93 Yes a fix over in validated-changeset with a simple test case seems prudent! Would happily accept.

[betocantu93]

Two more issues on my mind: 1.

Stumbled with another bug, when you try to changeset.get('super.deep.key') where content doesn't have such structure, it fails on this line because we are trying to getDeep on undefined. https://github.com/validated-changeset/validated-changeset/blob/d9e54bea45d0ea88d4de7e9faf39733c050c755b/src/index.ts#L963

Not exactly sure how to handle that case inside the get function

2. I also noticed an inconvenience. As we are notifying the [CONTENT] well, it will trigger all the observers in the wrapped model, which is ok for most scenarios, i.e a custom route to edit a model, but if by any UX you have both the model and the changeset rendered (think of a table with model rows on the left, with a flyout changeset editing) it will trigger whatever observers you might have on the changeset and the model row, so you might have unexpected UX...

What do you think about the second point? would it make this approach non ideal?

My counter proposal is to have a tracked mirror/fake structure that we will be notified and (deeply) setted just for observavility purposes, instead of notifying the content. We just have to be sure (I think 🤔) that we consume tags per get/set, with that octane getters should work and classic computeds should work, and for addObserver, we might have to advise to hook em to this mirror... i.e. `Ember.addObserver(changeset._mirror, 'some.nested.prop');

[snewcomer]

Fixing 1 in #590! Thanks!

Regarding 2, it sounds like we have to get really really granular on our KVO notifications. Just for some clarification, is the problem solely with addObserver? Or are there other cases that don't work.

Overall, we do need to get this right as you have pointed out - notifyPropertyChange just doesn't work right now with nested paths (or perhaps does but fires to broadly)

[betocantu93]

thanks for the fix, it was so simple!, I thought some sorcery would be needed haha.

Just for some clarification, is the problem solely with addObserver? Or are there other cases that don't work.

I think Ember addObserver is just complex for changesets. There was also an issue when you wrapped an object with proxy values, for example, ember-m3 Megamorphic Model, which is just a proxy that resolve values lazily per access, but I can't truly remember, i'll have to revisit. But that made me use another pattern for our M3 models use case.

I guess we could add a custom addObserver.

pseudo: changeset.addObserver('my.super.nested.key', this.callback)

//Ember-changeset
addObserver(path, callback) {
    setDeep(this.mirror, path, true); //We actually dont care about the value, we just need this "notifiable" structure
    addDeepObserver(this.mirror, path, callback); //add the observer to the ancestor
}

[betocantu93]

I was thinking in something like this in pseudocode:

//ember-changeset
new Proxy(obj, {
   get(target, path) {
       setDeep(target[MIRROR], path, true); 
       .... continue default
   },

  set(target, path, value) {
      setDeep(target[MIRROR], path, value /*I think we don't care about the value*/);
      ....continue default
  }
})

setProperty(){
 ....
deepNotifyPropertyChange(this[MIRROR], path)
}

We are basically building lazily this notifiable pojo, not entirely sure if it's compatible with native getters and computeds, or if the mirror should be marked as tracked and consume the tags per set/get, so when notified, native "octane" getters should recompute, but for classic computeds probably mirror dependencies should be manually added.

[betocantu93]

How does the changeset-get helper gets to recompute 🤔

[betocantu93]

The problem with native getters, tracked and ember-changeset is that every consumed tag is a dependency, for example: