Logging inconsistent between object/array for $destroy

[katowulf]

(feedback quoted from a user)
I just updated to AngularFire 1.0.0. I've added $destroy events to a
$firebaseArray and a $firebaseObject. I noticed that the $destroy on a
$firebaseArray does a debug log (destroy called for FirebaseArray: .....).
However no such log occurs for the $firebaseObject.

This has me wondering:

• Should the debug log be removed for firebaseArray? or
• Is the debug log missing for $firebaseObject? or
• Is $destroy not actually working for $firebaseObject?

[krokhale]

Noticed this too. And i think an edge case here maybe.

$scope.currentMembers = $firebaseArray($scope.chatRefs.roomMembers.child(roomId))

Firebase looks like:

roomMembers: {'firebaseGeneratedId' : {}, 'firebaseGeneratedId' : {}, ......}

If there is just one member and he leaves the room, roomMembers should become null as expected. I remove members using where i just set the member id to null:

$scope.removeMember(memberFirebaseGeneratedId)

Right after that i want currentMembers to sync with a set of new room members, so i destroy the currentMembers:

$scope.currentMembers.$destroy() if $scope.currentMembers

And this is when my console blows:

TypeError: Cannot read property '$getRecord' of null
at angularfire.js:1
at angularfire.js:1
at Object.forEach (angular.js:323)
at g (angularfire.js:1)
at angular.js:16223
at completeOutstandingRequest (angular.js:4905)
at angular.js:5285

[mikepugh]

My preference would be to have debug log added to $firebaseObject. Since $destroy can lead to various permissions issues, it's good to have the notification that it's happening. For prod use, angular makes it easy enough to disable debug messages via $logProvider.

[jwngr]

I'd actually vote to remove the $log.debug() call in $firebaseArray.$destroy(). I don't think we ever intended it to ship with it there. I'm generally against unnecessary "on-by-default" debugging like this and think we should avoid it.

@krokhale - It sounds like you found a separate issue. Can you please open a new issue and provide a full repro of the issue. You can fork my AngularFire Plunker Seed so you don't have to set up a lot of the boilerplate for the repro. Please use a demo Firebase (see the URL I included in the script.js file of the Plunker) and put all the data in there that is required to repro the issue.

[katowulf]

@jwngr it will be much easier to deactivate these in Angular (which is a one-liner) than to $extend/decorate in order to add them for development. I'm in favor of keeping them.

[katowulf]

Also, this works as-is in the latest release. I couldn't repro this error. Both $firebaseObject and $firebaseArray log errors for permission denied.

[jwngr]

The main thing I don't like about this is that $log.debug() is by default turned on. So you actively have to know we log these things in order to turn them off. I also don't see why logging during destroy is any more important that any other event in AngularFire. It seems super arbitrary.

Also @katowulf, where does the $firebaseObject logging happen? I only see a single place in the code where we have $log.debug() and it's here, in the $firebaseArray code.

This bug wasn't about permission denied errors (which are out of our control since the Firebase library itself logs them), it was about extraneous logging when $firebaseArray was destroyed. I still am strongly in favor of removing it.

[katowulf]

Only one other place, in the $$error method: https://github.com/firebase/angularfire/blob/86ea97e8e5a5cbf1691f422b64467e654976686b/src/FirebaseArray.js#L394

[katowulf]

So you actively have to know we log these things in order to turn them off.

$log is a standard service of the Angular framework and a recommended method for attaching debugging. Turning it on or off, or changing verbosity, is as simple as turning Firebase logging on or off. It's their purview to turn debug logging on by default; they've decided that it's useful for development and simple enough to turn off for production. I don't feel this is an AngularFire issue and disagreement with the on/off by default should be taken up with the Angular team. It feels trivial in either case.

Whether or not we have a debug statement in the destroy method is not worth much argument. It could stay or go. The $destroy logging is of interest since destroy can get called in different ways and may not be immediately intuitive; it could be helpful for figuring out why things mysteriously quit synchronizing (hopefully we'd get an error as well, so again minor).

However, logging of errors in $$error is a bit more important. All of the common errors are authentication and security related, and all will require some output in order to troubleshoot. So I feel like on by default, and even on in production, are very appropriate for these. I don't imagine there's too much disagreement about error logging, and since it can be turned off easily, should be kept.

[jwngr]

I'm totally on board keeping $log.error() around. No arguments from me there. But this bug is about the $log.debug() inconsistency between $firebaseArray and $firebaseObject (see your very first comment). That inconsistency is still around in master and should go away with the next release. I suggest we resolve it by removing the debug logging from $firebaseArray instead of adding debug logging to $firebaseObject. What say ye?

[katowulf]

I say ☃.

[jwngr]

Yay! Agreement 😀