FirebaseExtended / angularfire

AngularJS bindings for Firebase
MIT License
2.73k stars 632 forks source link

Data changes are not stored if modified in Controller definition #46

Closed abeisgoat closed 11 years ago

abeisgoat commented 11 years ago

It appears that angularFire does not push updates to Firebase if the data is not modified from a method on $scope. I think this has to do with $apply not being called. For example, take this code.

app.controller('Home', ['$scope', 'angularFire',
function Home($scope, angularFire) {
    var url = 'https://<firebase>.firebaseio.com/';

    var promise = angularFire(url + '/items', $scope, 'items', []);

    promise.then(function() {
      // This new entry shows in rendered template, but doesn't push to firebase
      $scope.items.push({name: "Firebase", desc: "is awesome!"});

      $scope.removeItem = function() {
        $scope.items.splice($scope.toRemove, 1);
        $scope.toRemove = null;
      };
    });
}
])

As far as I know, this isn't really a bug as much as an issue with how Angular is setup and how I'm using it. From what I've read, when modifying data outside of the typical angular "loop" we need to call $apply for the changes to be reflected. This happens, for example, when changing data on $scope through dev tools. I think that is what is happening here. Since data isn't typically modified at this time, $apply is not called. When I call $apply manually or trigger it with a click, then the updates are stored as expected.

I don't believe this is a bug as much as misuse of angular on my part, but regardless it is an issue which I suggest be addressed in the docs as to avoid further confusion for others!

anantn commented 11 years ago

I wonder if we could do something like call the promise callback wrapped in an $apply so that any changes made to $scope inside it would be picked up by Angular. Worth investigating...

katowulf commented 11 years ago

It would be nice if Angular would just provide a darn set method on $scope : (

abeisgoat commented 11 years ago

I just took a crack at patching this, I may (probably) have broken something in the process. The issue seems to be related to the _initial flag on angularFire and not $apply as I suspected. In the current branch the first call of angularFire._watch is ignored because of this flag. I'm not sure why this behavior exists (that's why I suspect my patch might break something), but I know removing it fixed this issue. I assume that because the $scope.items.push happens before the initial call of _watch the update is ignored and assumed to be part of the initial loading of data from Firebase, when in fact it is not.

anantn commented 11 years ago

We ignore the call to ._watch the first time we set data into $scope. - I'd like to see if there's a way for us to guarantee that _initial is set to false before we resolve the promise. I thought that's what the behavior was, but it doesn't seem like that's the case.

abeisgoat commented 11 years ago

What issue does the ._initial related code exist to solve? What problem exists when _watch is run the first time we set data to the $scope?

anantn commented 11 years ago

It exists to make sure we ignore _watch being triggered when we set the initial data to $scope. We could alternatively refactor the code that the watch isn't attached until after the initial data is loaded.

mck- commented 11 years ago

@abeisgreat you say that when you call $apply manually it works? Could you elaborate?

I tried wrapping $scope.items.push({name: "Firebase", desc: "is awesome!"}); in a $scope.$apply , but it still doesn't push up to Firebase

abeisgoat commented 11 years ago

@mck- I'm sorry I wasn't quite clear and was slightly incorrect in my diagnosis aswell, What I should have said was that pushing new data after the promise had been fulfilled appears to push any old unsynced data. I now realize this issue isn't related to $apply as much as I thought, but instead it has to do with _watch and the timing with the promise. The work around to this is to only write data to Firebase inside of the promise response, never before, if that helps.

mck- commented 11 years ago

Thx @abeisgreat -- when you say to write data to Firebase inside of the promise response, do you mean inside the function of promise.then? When I make changes to the $scope on initial loading, it does not sync up -- just like you said, it only sends up after the next update.

Sorry, I'm still unclear about your work around...

So right now, I'm avoiding any $scope changes upon initial loading and only trigger them with an action -- otherwise the local $scope and FB's scope gets out of sync -- but obviously it would be much better if it were synced upon initial loading too.

olostan commented 11 years ago

Found same issue and spent about hour to understand that it is not my fault.... I think until fixed this text should be mentioned in Readme:

You object is not tracked when modified inside of then promise...

Here is Plunkr that illustrate this issue: http://plnkr.co/edit/p1pHSzYTS7MqOXmNqID9?p=preview

olostan commented 11 years ago

The only workaround what I've found for now its to wrap code that modify object, fetched from Firebase into $timeout

anantn commented 11 years ago

It's best to only modify $scope after the function .then() of the promise has been called. We're looking into what we can do to retain changes made to $scope before the promise is resolved. It's a little tricky because we can't distinguish between actual changes and initial state. For example:

$scope.val = [];
angularFire(url, $scope, "val").then(function() {
  $scope.val.push("baz");
});

is different, but cannot be distinguished from:

$scope.val = ["foo", "bar"];
angularFire(url, $scope, "val").then(function() {
  $scope.val.push("baz");
});
// User deleted two items.
$scope.val = [];

Would love to hear about creative solutions!

olostan commented 11 years ago

Just can't understand there is a check for _initial... I've just remove it as suggested @abeisgreat (commented lines from 96 till 99) and seems its works great: http://plnkr.co/edit/p1pHSzYTS7MqOXmNqID9?p=preview Also changed $timeout to $apply when new value is comming from FB.

In general I think this should be quite straighfull: Watcher is been added after resolving promise, so local value is been ignored before resolving. After the first value come from FB,

  1. this value is set to scope
  2. watcher is been added
  3. promise is been resolved

so after it all changes to value is been tracked by watcher having local and FB values in sync.

In my plunker I've altered scope value after calling AngularFire but before value is loaded and seems everything works as expected - local value is been watched only when promise resolved and just after it (not later).

katowulf commented 11 years ago

Why was $timeout changed to $apply? Were you getting some sort of error or issue with $timeout? There are a lot of implications of using $apply--for instance, it throws an exception if a compile is currently in progress, which is avoided by using $timeout, so I'd like to see a justification for changing it.

mck- commented 11 years ago

The only workaround what I've found for now its to wrap code that modify object, fetched from Firebase into $timeout

@olostan the $timeout workaround, is that only working when you remove lines 96 to 99? Or is it a workaround even with the original library? Once I uncomment those lines, it still doesn't work

Seems to me that @abeisgreat 's approach works fine without any $timeout magic

olostan commented 11 years ago

@mck- $timeout workaround for original version of library. With commenting lines with _initial no any workaround needed.

olostan commented 11 years ago

@katowulf Actually this is what $apply is intended for: if you need to modify some data from 3rd party library, that is not aware about AngularJS event loop, you should wrap modification code into $apply.

Actually $tiemtout without time parameter just wrap your callback in $apply in case if it is called non from $digest loop.

Using $timeout as a hack to have your code $applied is little bit strange... it shows that there is some inconsistency in code - normally is should be clear: All code that runs inside AngularJS event loop should be called directly, and code that is outside should be $applied.

And moreover, when AngularJS would utilize Object.observe features, wrapping into $timeout would be even more strange...

gdiazlo commented 11 years ago

I was wondering if this is related to a problem I have. The controller below feeds a view with a table and a form. The relevant part of the table is:

<tr ng-repeat="obj in tab.col | filter:filterObj" ng-click="select(obj)" ng-class="isSelected(obj)">

If a row gets clicked, the form displays the details of the obj (its model is tab.obj), and you can edit it. The table gets the update appropiately, but firebase does not get any updates.

define(['app'], function(app) {
  app.controller('LinksCtrl',['$scope', '$filter', '$http','angularFire','angularFireAuth', function($scope,$filter,$http, angularFire, angularFireAuth) {
    $scope.links = {};
    $scope.tabs = [];

    var url = "https://<url>.firebaseio.com/links";
    angularFireAuth.initialize(url, {scope: $scope, name: "user", path: "/"});
    var promise = angularFire(url, $scope, 'links', {})

    $scope.select = function (item) {
      for(i in $scope.tabs) {
        var tab = $scope.tabs[i];
        if (tab.active) {
          tab.obj = item;
          if (tab.obj.selected) {
            tab.obj.selected = ! tab.obj.selected;
          } else {
            tab.obj.selected = true;
            // $scope.$apply();
          }
        }
      }
    };

    $scope.isSelected = function(obj) {
      if (obj.selected && obj.selected === true) {
       return "active"; 
      }
      return "";
    }

    promise.then(function() {

      $scope.tabs.push({
        title: 'links',
        disabled: false,
        active: false,
        col: $scope.links.open, //this is an array of objects
        obj: {}
      });

      $scope.tabs[0].active = true;

      for(i in $scope.tabs) {
        var tab = $scope.tabs[i];
        tab.actions = app.actions();

        // app.actions returns an array with more objects like the following doing CRUD and other basic stuff
        tab.actions.push({
          name: 'Delete link',
          icon: 'icon-minus',
          cond: function(tab) { if ('selected' in tab.obj) { return tab.obj.selected}; return false; },
          func: function(tab) {
            // splice tab.col or whatever modification
          }
        });
      };
    });
  }]);

});

How can i make something like this work? should a turn to manual sync via collections? debugin with console.log shows the object obj has what it is supposed to have in all stages of the code, it is just that the firebase does not get the updates.

thanks

abeisgoat commented 11 years ago

@gdiazlo No where in your code do you write to $scope.links (the angularFire field). I don't think your problem is related to this bug. If you'd like more help related to this, feel free to send me an email at abeisgreat@abeisgreat.com and I'll help you sort out your code.

gdiazlo commented 11 years ago

@abeisgreat thanks a lot, i think i solved the problem, still don't know why it is this way, but seeems it is. Let me explain.

In the controller i have a structure called tab, which contains data related to each tab in the view (a bootstrap tab in the view), like title, col(lection), status (active, disabled), and selected obj.

This leads to a $scope tree like:

level 0 $scope ---- in the controller tab[0].col = $scope.links

level 1 $scope ---- in the view ng-repeat="tab in tabs"

level 2 $scope ---- in the view ng-repeat="obj in tab.col"

Modifications to obj, get to level 1, but not to level 0, so i end up changing the level 2 repeat like this:

ng-repeat="obj in links"

This case has only one tab, but i have cases with multiple tabs, so i changed the ng-repeat in those like:

ng-repeat="obj in class[tab.title]"

This works for all cases i have, but i thought that assigments in javascript like tab[n].col = $scope.links was puting a reference into tab[n].col, but seems that's not the case, or the $scope propagation only works one level up :-?

thanks again,

adam-g-pullen commented 11 years ago

I have had a crack at trying some form of "Write on Create"

var ref = angularFire(fbURL + "/users/" + userId, $scope, 'mainUser', {
    id : userId,
    name : "Set directly during creation"
});

Which will allow new objects/arrays to be written directly to Firebase during the registration of the endpoint

The backing _watch code has been changed to this

// Watch for local changes.
_watch : function($scope, name) {
    var self = this;
    self._unregister = $scope.$watch(name, function() {
        // useWriteOnCreate will be used to control two branches
        // firstly: checking the _initial var, if the object/array has props 
        // then we will consider this to be a write on create request
        // secondly: when checking the difference between the local
        // object and the remote object, since the first request they will "match"
        var useWriteOnCreate = false;
        // When the local value is set for the first time, via the
        // .on('value')
        // callback, we ignore it.
        if (self._initial) {
            // this is the initial request, does the supplied object "type" contain properties?
            for ( var prop in self._remoteValue) {
                // yes, the existence of one prop is good enough to dictate that the user wishes
                // their new object to be created right now
                useWriteOnCreate = true;
                break;
            }
            // remove _initial flag
            self._initial = false;
            // has the object any props, if not then proceed with original ignore
            // otherwise continue on
            if (!useWriteOnCreate)
                return;
        }
        // If the new local value matches the current remote value, we don't
        // trigger a remote update.
        var val = JSON.parse(angular.toJson(self._parse(name)($scope)));
        if (angular.equals(val, self._remoteValue)) {
            // even though the vals are the same (as will be in the case of registering a new endpoint)
            // if this is the initial request and props were set continue on with the write up to
            // firebase
            if (!useWriteOnCreate)
                return;
        }
        self._fRef.ref().set(val);
    }, true);
},

The code could be cleaned up somewhat, i have done some testing with object, simple testing with array's and no testing with other datatypes.

I understand the need for the _initial so it should stay in.

My use case is this:

I have an object that is written to by the system, the user never needs to write to it, this causes angular to never update the object and thus angularFire is also not invoked.

anantn commented 11 years ago

Hey everyone! The latest version of angularFire doesn't require you to wait for the promise to be resolved and local changes will be merged with remote data when it arrives. I believe that addresses this issue, please open new tickets if you have trouble using the new version.