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.

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

Using observe-js ObjectObserver instead of Object.observe #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

daviddeutsch
Copy link

Unless I'm missing something here, this should be the way to do it, anyhow.

@daviddeutsch
Copy link
Author

Alright, seems like it wasn't so simple. The callback interface is different. As noted in the observe-js ObjectObserver intro, the callback should have added, removed, changed, getOldValueFn as arguments. When I assign these to an array like so:

function onObjectObserved(added, removed, changed, getOldValueFn) {
  var changes = [{
    added: added,
    removed: removed,
    changed: changed
  }];
  // ...

It seems to work. Will test some more.

@daviddeutsch
Copy link
Author

Alright, had to make two further adjustments to get this to work. Only thing I'm not sure about is this bit:

function onObjectObserved(added, removed, changed, getOldValueFn) {
  var changes = [{
    added:   added,
    removed: removed,
    changed: changed,
    index:   getOldValueFn(context.key)
  }];

because it uses context.key and a couple lines below, there is an if block for when we don't have that set:

if (!context.key) {
  angular.forEach(changes, pushSplice);
  callback.call(context, splices);
}
else {
  callback.call(context, changes);
}

For my own use case, I always put in the context key and I'm not sure how I would fix this if there wasn't one. Exercise for the repo owner, I guess 😉

@daviddeutsch
Copy link
Author

(apologies about the merge derp)

@DaftMonk
Copy link

pinging @jeffbcross, any plans to merge this?

@daviddeutsch
Copy link
Author

I'm also wondering about the status in general. I'll be using omnibinder rather heavily pretty soon, so either this project gets picked up again or I will have to do a fork. I'm also slightly concerned about the name, to be honest - it's clearly inspired by the OmniGroup stuff, so I'm not sure whether the name is a good idea.

@daviddeutsch
Copy link
Author

Just wanted to mention that I've started working on my own version, for now called "octobinder" over here: https://github.com/daviddeutsch/octobinder

The main differences are adopting the PRs from this repo here, cleaning up the code a bit and moving a lot of stuff to a promise-based approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants