Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spike for prop notification #586

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
+2,150 −1,843
Open

spike for prop notification #586

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b38d79c
spike
betocantu93 Apr 7, 2021
a794db2
fix
betocantu93 Apr 7, 2021
b780c13
fix
betocantu93 Apr 7, 2021
4d1469a
fix
betocantu93 Apr 8, 2021
907ee21
fix notifying for changes
betocantu93 Apr 13, 2021
b232cd9
add test
betocantu93 Apr 13, 2021
65b528b
improve notif
betocantu93 Apr 16, 2021
a186905
fix tests
betocantu93 Apr 16, 2021
68c426e
add assertion for undefined
betocantu93 Apr 23, 2021
4ad58ce
update lock
betocantu93 Apr 24, 2021
a93ddeb
Merge branch 'master' of https://github.com/poteto/ember-changeset in…
betocantu93 Apr 24, 2021
a345077
bump node
betocantu93 Apr 24, 2021
17380df
fix
betocantu93 Apr 24, 2021
f680506
do not trigget proxy set trap
betocantu93 Apr 24, 2021
d59a4ec
Merge branch 'master' of https://github.com/poteto/ember-changeset in…
betocantu93 May 9, 2021
218ff11
update yarn
betocantu93 May 9, 2021
b5f30fe
fix: update validated-changeset
betocantu93 Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions addon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,31 +32,31 @@ function maybeUnwrapProxy(o) {
return isProxy(o) ? maybeUnwrapProxy(safeGet(o, 'content')) : o;
}

function tryContent(ctx) {
function getContent(ctx) {
return ctx._content ? ctx._content : ctx.content ? ctx.content : ctx;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

function deepNotifyPropertyChange(changeset, path) {
let paths = path.split('.');
let maybeDynamicPathToNotify = null,
lastPath = paths.pop(),
current = changeset,
current = getContent(changeset),
i;

//If the path doesn't exists previously exist inside the CONTENT, this is a dynamic set.
let existsInContent = safeGet(changeset[CONTENT], path) !== undefined;

for (i = 0; i < paths.length; ++i) {
const curr = current[paths[i]];
if (existsInContent && curr && curr.content === curr.content) {
current = current[paths[i]];
const curr = safeGet(getContent(current), paths[i]);
if (existsInContent && curr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this fail if curr is falsey?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping!

current = curr;
} else {
maybeDynamicPathToNotify = paths[i];
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Does the if else blocks need to be switched? If != undefined, we want to notifyProperty change at that level?
  2. 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?

Copy link
Contributor Author

@betocantu93 betocantu93 Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
const pathToNotify = maybeDynamicPathToNotify ? maybeDynamicPathToNotify : lastPath;
notifyPropertyChange(tryContent(current), pathToNotify);
notifyPropertyChange(current, pathToNotify);
}

export class EmberChangeset extends BufferedChangeset {
Expand Down