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

Make $-prefixed properties of $firebaseObject non-enumerable #579

Closed
ExplodingCabbage opened this issue Mar 6, 2015 · 16 comments
Closed

Comments

@ExplodingCabbage
Copy link

Presently, the $$conf, $id and $priority properties of a $firebaseObject are normal, enumerable properties. This is inconvenient if I want to loop over the object's properties with a for .. in loop or something like LoDash's _.each() method.

Currently I work around this with code like

_.each(someFirebaseObject, function (value, key) {
    if (key.lastIndexOf('$', 0) === 0) {
        // Filter out AngularFire methods on object
        return;
    }
    else {
        // do something
    }
});

but this is ugly, and the ugliness is greater still in circumstances where what I'd really like to do is something like an _.map call.

It would be more convenient if these properties were created with Object.defineProperty to be non-enumerable.

Considerations:

  • This will break IE 8 support (if the library isn't broken in IE 8 already)
  • ... but you're officially not supporting Angular 1.2.x any more, which was the last version of Angular to support IE 8, so you officially don't support IE 8 anyway
  • Concerns about IE 8 aside, this is technically a breaking change, albeit a minor one that probably won't affect many people
  • Angular are likewise considering making their $foo properties non-enumerable (see what is currently the final post at ngRepeat: property names starting with "$" will not be rendered angular/angular.js#6266) but may or may not follow through on this

I advocate making the change, but you may see things differently.

@katowulf
Copy link
Contributor

katowulf commented Mar 6, 2015

Use angular.forEach()

@ExplodingCabbage
Copy link
Author

Ah, nice. Didn't realise that filtered out $-prefixed properties; it's not documented as doing so, but I just tested it and you're quite right.

This issue still causes slight nuisance when I want to do a map, since there's no analogous angular.map method. I leave it to you to decide whether to close this or whether that remaining point makes it worth addressing.

@katowulf
Copy link
Contributor

katowulf commented Mar 6, 2015

Yep, I didn't mention it, but we do appreciate the feedback (at ng-conf and a bit distracted). I was just pondering what action item to take on this.

@katowulf
Copy link
Contributor

katowulf commented Mar 6, 2015

For what it's worth, we've already done this for $$conf. If you are seeing $$conf, then lodash/underscore must be bypassing the hasOwnProperty() check?

@ExplodingCabbage
Copy link
Author

Double-checks... nope, I don't see $$conf. My bad.

There is one other point that might be worth considering here: if angular/angular.js#6266 were ever to get implemented, wouldn't it break most projects using current versions of AngularFire in a spectacular way? It might be worth chiming in there, and/or making these properties non-enumerable to make sure that such a change can't cause an incompatibility with AngularFire.

@katowulf
Copy link
Contributor

katowulf commented Mar 6, 2015

Since $firebaseArray is a real array, it's not really susceptible here--it will pass forEach. $firebaseObject would be a different story, but I'm having trouble coming up with a valid use case for using ng-repeat on a non-collection, so I'm not sure if this has any priority. Also, this should probably be a separate issue, considering that we're branching the discussion quite a bit beyond the scope of non-enumerable ids.

@ExplodingCabbage
Copy link
Author

A $firebaseObject may be a collection though - it may be an arbitrary mapping of keys to values, rather than a structured object with fixed keys. Just as an object in JavaScript may be either of those things. I've certainly used a $firebaseObject in that way.

@ExplodingCabbage
Copy link
Author

Example context: having a $firebaseObject whose keys are user ids, and whose values are information about the user. Then on some admin page we have a list of all users using an ng-repeat. angular/angular.js#6266 would screw up this use case.

@katowulf
Copy link
Contributor

katowulf commented Mar 6, 2015

Unless we make the $ variables non-enumerable, presumably. Let me ponder that. But really, if it's a list of users, $firebaseArray() would be more appropriate.

@katowulf
Copy link
Contributor

Decided not to take any action here. Making this properties non-enumerable will effectively make them harder to discover for those who don't like to read the docs and would rather poke around in the debugger. Since we expect collections to be handled as part of $firebaseArray, I've decided to punt on this and leave $id and $priority simple to iterate. angular.forEach() provides a nice simple workaround for avoiding them.

@typeofweb
Copy link

@katowulf angular.forEach() doesn't filter $-prefixed properties. Maybe it used to but it's never been documented and certainly it doesn't work in Angular 1.4 (anymore?)

Is there any other way to get just the data from $firebaseObject instance? Using $firebaseObject is really pain in the neck now since we have to filter out all the $-prefixed fields:

const obj = $firebaseObject(firebaseRef.child());

angular.forEach(obj, (val, key) => {
    if (String(key).charAt(0) === '$') {
        return;
    }
    
});

@jamestalmage
Copy link
Contributor

To my knowledge it never has, but it does filter using hasOwnProperty, and since pretty much every $ prefixed property is defined on the prototype - all those get filtered out. Only exception would be $priority.

It is not well documented, but there is $firebaseUtils.toJSON(obj), and $firebaseUtils.each(obj, fn) - play with both and see if they meet your needs. (note: toJSON produces an object stripped of key values that are illegal in firebase. It does not produce a JSON string).

@typeofweb
Copy link

@jamestalmage thanks a lot, $firebaseUtils solves all my problems. It's a shame I didn't check it before.

@sunnypatel
Copy link

Has this been fixed? Or does anyone have a workaround?

@jwngr
Copy link

jwngr commented Aug 24, 2015

Does @jamestalmage's suggestion above of using $firebaseUtils not solve your problem? If so, please provide some more details.

@jwngr jwngr added the wontfix label Aug 24, 2015
@1dolinski
Copy link

@jamestalmage Thank you, this helped me as well.

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

No branches or pull requests

7 participants