spike for prop notification #586

betocantu93 · 2021-04-07T16:39:05Z

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

Reduce property notif change with tracked-built-ins #517 Changeset changes array is not cleared when no changes exist #585 property change notification too broad #499 Modifying a buffered property of a changeset invalidates all buffered properties #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 · 2021-04-09T19:42:20Z

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

betocantu93 · 2021-04-09T20:29:23Z

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 · 2021-04-13T22:13:03Z

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

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.
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')
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.

snewcomer

This PR looks like it is making progress!

snewcomer · 2021-04-15T03:59:02Z

addon/index.js

+      current = current[paths[i]];
+    } else {
+      notifyPropertyChange(tryContent(current), paths[i]);
+    }


If I'm understanding this correctly, we want to traverse down the object to the leaf key, calling notify property change as we go? Or do we want to reduce property notifications (as indicated in the issue) and only want to notify at the leaf key in paths?

Two questions.

Does the if else blocks need to be switched? If != undefined, we want to notifyProperty change at that level?

Does it make more sense to start at the leave and traverse up? Or rather, start at the last key in paths and iterate backwards?

Rewrote my last comment as my understanding changed.

The idea is to traverse down to find the deepest defined ancestor.

example 1.
Take: key as foo.bar.baz and data as { foo: { bar: {} } }

The output should: notifyPropertyChange(data.foo.bar, 'baz')

example 2.
Take: key as foo.bar.baz and data as { }, everything undefined

The output should: notifyPropertyChange(data, 'foo')

example 3.
Take: key as foo.bar.baz and data as { foo: {} }, so bar undeefined

The output should: notifyPropertyChange(data.foo, 'bar')

I think this could be optimized like this:

for (i = 0; i < paths.length; ++i) { if (current[paths[i]] != undefined) { // we can still go deep current = current[paths[i]]; } else { //we just found that its a dynamic set, so we notify here and stop, no point in going further. notifyPropertyChange(tryContent(current), paths[i]); return; } } //We didn't stop, we haven't notified, and we went the deepest as possible, so we notify there. notifyPropertyChange(tryContent(current), lastPath);

I think its ok to start from the top, because we can early notify and return

betocantu93 · 2021-04-15T17:29:54Z

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

betocantu93 · 2021-04-16T01:08:41Z

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.

tests/unit/changeset-test.js

snewcomer · 2021-04-16T18:18:49Z

tests/unit/changeset-test.js

+    assert.equal('Jack', res, 'observer fired when setting value some.really.nested.path');
+    changeset.rollback();
+    assert.equal(undefined, res, 'observer fired with the value some.really.nested.path was rollback to');
+  });

  test('can update nested keys after rollback changes.', async function (assert) {
    let expectedResult = {


#585

Do we have a test for this issue so we can close it out when merged?

ping! Lmk what you think about adding this test. Lots of great work in this PR.

sorry for the delay, I ran that repo, I think it has a bug on the example itself, it uses

@value={{not (changeset-get this.changesetObj contextKey)}} @onToggle={{action (changeset-set this.changesetObj contextKey) (not (changeset-get this.changesetObj contextKey))}}

I think that action helper is actually(?) mutating the CONTENT, somehow(?) and thus it actually have changes

but I just ran that branch with a few changes to the example and its working!

@value={{changeset-get this.changesetObj contextKey}} @onToggle={{changeset-set this.changesetObj contextKey (not (changeset-get this.changesetObj contextKey)) }}

And so, I think... no further test need to be added? 🤔 I mean there are a few tests already checking for changes emptied after setting back the initial wrapped value? gonan double check

Hmm I ran this again and the bug still exists, but I'm unable to track why it seems somehow sets are applied to the this[CONTENT] without executing the changeset using changeset-set.

…to spike-fix-prop-notification

betocantu93 · 2021-04-24T15:13:23Z

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.

snewcomer · 2021-04-24T16:28:59Z

package.json

@@ -75,12 +75,12 @@
    "release-it-lerna-changelog": "^1.0.3"
  },
  "engines": {
-    "node": "10.* || >= 12.*"
+    "node": ">= 12.*"


This will be a major version bump I believe. What was the specific error? I thought v10 had another 6 days until EOL.

snewcomer · 2021-04-24T16:40:13Z

addon/index.js

+
+  for (i = 0; i < paths.length; ++i) {
+    const curr = current[paths[i]];
+    if (existsInContent && curr && curr.content === curr.content) {


curr will fail for falsey values, correct?

Do we want to use getPrototypeOf and walk the chain checking for getOwnPropertyDescriptor? Or perhaps we just directly check hasOwnProperty on the object.

Yeah, I think that makes sense

betocantu93 · 2021-04-24T17:11:39Z

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 · 2021-05-03T13:44:39Z

addon/index.js

-    if (existsInContent && curr && curr.content === curr.content) {
-      current = current[paths[i]];
+    const curr = safeGet(getContent(current), paths[i]);
+    if (existsInContent && curr) {


Will this fail if curr is falsey?

snewcomer · 2021-05-03T13:50:37Z

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

snewcomer · 2021-05-09T13:35:58Z

addon/index.js

@@ -32,6 +32,33 @@ function maybeUnwrapProxy(o) {
  return isProxy(o) ? maybeUnwrapProxy(safeGet(o, 'content')) : o;
 }

+function getContent(ctx) {
+  return ctx._content ? ctx._content : ctx.content ? ctx.content : ctx;


Is _content meant to capture from the Changeset and content from an ObjectProxyNode?

Yes, that's correct.

Sry for stringing this along but I do want to get a solution in. I'm learning from your work to understand the issue better.

So is it best to say, not only does notifyPropertyChange not work for nested keys (this is obvious to me), but also we are notifying on the wrong object? Meaning notifyPropertyChange(this, key) leads to the wrong/no expected behaviour as well? I wouldn't have through this would have work for an ObjectProxyNode (non Ember object, etc)

betocantu93 · 2021-05-09T18:03:47Z

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

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 · 2021-05-09T20:18:14Z

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 · 2021-05-09T21:12:38Z

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 · 2021-05-09T21:21:34Z

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.

…to spike-fix-prop-notification

betocantu93 · 2021-05-09T22:37:37Z

How does the changeset-get helper gets to recompute 🤔

betocantu93 · 2021-05-09T23:01:09Z

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

This was referenced Apr 7, 2021

Reduce property notif change with tracked-built-ins #517

Open

property change notification too broad #499

Open

snewcomer assigned betocantu93 Apr 15, 2021

snewcomer added bug enhancement labels Apr 15, 2021

snewcomer reviewed Apr 15, 2021

View reviewed changes

betocantu93 added 8 commits April 15, 2021 20:30

spike

b38d79c

fix

a794db2

fix

b780c13

fix

4d1469a

fix notifying for changes

907ee21

add test

b232cd9

improve notif

65b528b

fix tests

a186905

betocantu93 force-pushed the spike-fix-prop-notification branch from b44e85e to a186905 Compare April 16, 2021 01:30

snewcomer reviewed Apr 16, 2021

View reviewed changes

tests/unit/changeset-test.js Show resolved Hide resolved

snewcomer reviewed Apr 16, 2021

View reviewed changes

betocantu93 added 4 commits April 23, 2021 18:01

add assertion for undefined

68c426e

update lock

4ad58ce

Merge branch 'master' of https://github.com/poteto/ember-changeset in…

a93ddeb

…to spike-fix-prop-notification

bump node

a345077

fix

17380df

snewcomer reviewed Apr 24, 2021

View reviewed changes

do not trigget proxy set trap

f680506

betocantu93 mentioned this pull request Apr 24, 2021

Changeset changes array is not cleared when no changes exist #585

Open

snewcomer reviewed May 3, 2021

View reviewed changes

snewcomer reviewed May 9, 2021

View reviewed changes

This was referenced May 9, 2021

[Bugfix]: no underlying content path to key that was modified adopted-ember-addons/validated-changeset#116

Merged

[Bugfix]: get nested key when doesnt exist in underlying content #590

Merged

betocantu93 added 2 commits May 9, 2021 17:09

Merge branch 'master' of https://github.com/poteto/ember-changeset in…

d59a4ec

…to spike-fix-prop-notification

update yarn

218ff11

betocantu93 mentioned this pull request May 10, 2021

Tracked mirror approach for prop notifications #591

Open

fix: update validated-changeset

b5f30fe

snewcomer force-pushed the master branch from f0bf2fa to c5905be Compare April 18, 2022 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spike for prop notification #586

spike for prop notification #586

betocantu93 commented Apr 7, 2021 •

edited

Loading

snewcomer commented Apr 9, 2021

betocantu93 commented Apr 9, 2021

betocantu93 commented Apr 13, 2021 •

edited

Loading

snewcomer left a comment

snewcomer Apr 15, 2021

betocantu93 Apr 15, 2021 •

edited

Loading

betocantu93 commented Apr 15, 2021 •

edited

Loading

betocantu93 commented Apr 16, 2021 •

edited

Loading

snewcomer Apr 16, 2021

snewcomer Apr 23, 2021

betocantu93 Apr 23, 2021 •

edited

Loading

betocantu93 Apr 23, 2021

betocantu93 Apr 24, 2021

betocantu93 commented Apr 24, 2021

snewcomer Apr 24, 2021

snewcomer Apr 24, 2021

betocantu93 Apr 24, 2021

betocantu93 commented Apr 24, 2021 •

edited

Loading

snewcomer May 3, 2021

snewcomer May 9, 2021

snewcomer commented May 3, 2021

snewcomer May 9, 2021

betocantu93 May 9, 2021

snewcomer May 18, 2021

betocantu93 commented May 9, 2021 •

edited

Loading

snewcomer commented May 9, 2021

betocantu93 commented May 9, 2021 •

edited

Loading

betocantu93 commented May 9, 2021 •

edited

Loading

betocantu93 commented May 9, 2021

betocantu93 commented May 9, 2021 •

edited

Loading

spike for prop notification #586

Are you sure you want to change the base?

spike for prop notification #586

Conversation

betocantu93 commented Apr 7, 2021 • edited Loading

snewcomer commented Apr 9, 2021

betocantu93 commented Apr 9, 2021

betocantu93 commented Apr 13, 2021 • edited Loading

One problem with this approach

snewcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betocantu93 Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

betocantu93 commented Apr 15, 2021 • edited Loading

betocantu93 commented Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betocantu93 Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betocantu93 commented Apr 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betocantu93 commented Apr 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snewcomer commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betocantu93 commented May 9, 2021 • edited Loading

snewcomer commented May 9, 2021

betocantu93 commented May 9, 2021 • edited Loading

betocantu93 commented May 9, 2021 • edited Loading

betocantu93 commented May 9, 2021

betocantu93 commented May 9, 2021 • edited Loading

betocantu93 commented Apr 7, 2021 •

edited

Loading

betocantu93 commented Apr 13, 2021 •

edited

Loading

betocantu93 Apr 15, 2021 •

edited

Loading

betocantu93 commented Apr 15, 2021 •

edited

Loading

betocantu93 commented Apr 16, 2021 •

edited

Loading

betocantu93 Apr 23, 2021 •

edited

Loading

betocantu93 commented Apr 24, 2021 •

edited

Loading

betocantu93 commented May 9, 2021 •

edited

Loading

betocantu93 commented May 9, 2021 •

edited

Loading

betocantu93 commented May 9, 2021 •

edited

Loading

betocantu93 commented May 9, 2021 •

edited

Loading