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

fix _parseObject() that got broken because of angular/angular.js#6253 #320

Closed
wants to merge 1 commit into from

Conversation

urish
Copy link

@urish urish commented Jun 14, 2014

The behavior of angular.toJson() has changed in Angular.js 1.3.0.beta-11, and it no longer strips properties starting with $. This breaks $bind(), since it tries to sync the $id property of objects back to server and fails.

Fixed by manually stripping everything that starts with a $ in _parseObject().

@bendrucker
Copy link
Contributor

See discussion in angular/angular.js#6253

Wouldn't it be more direct to handle this behavior in toJSON? Would probably add a nice perf boost as well since we could avoid the stringify/parse cycle.

Your comment mentions:

We use toJson/fromJson to handle special cases, such as Date

What do you mean by that?

@bendrucker
Copy link
Contributor

Separate but worthwhile question for @katowulf — does Firebase support $property as a reference name? If not then just regexing out $-prefixed property names is fine. But if so, serialization shouldn't be blindly stripping everything that begins with a $, especially since Angular itself realized that was a mistake and is moving away from it.

There was borderline rioting in angular/angular.js#1463 from Mongo users who were sending around raw objects with $-prefixed props.

@katowulf
Copy link
Contributor

Currently, Firebase keys can't contain any of these characters (including all $ prefixed vars):

. (period)
$ (dollar sign)
[ (left square bracket)
] (right square bracket)

(hash or pound sign)

ASCII Control Characters (0-31 and 127)

@bendrucker
Copy link
Contributor

That makes things easier then — still should use toJSON for perf/maintainability reasons

@urish
Copy link
Author

urish commented Jun 16, 2014

Hi @bendrucker, first of all - plunker that reproduces the issue:

http://plnkr.co/edit/ozepSQdPvc9ItnUmnCzn?p=preview

If you edit the text in the input box, you will see that the data is not saved and you get the following exception on console:
Error: Firebase.set failed: First argument contains an invalid key ($id). Keys must be non-empty strings and can't contain ".", "#", "$", "/", "[", or "]".

If you switch to previous angular.js version (1.3.0-beta.10) then it works just fine.

@urish
Copy link
Author

urish commented Jun 16, 2014

About the Date comment - here is another plunker that shows the issue. It has a version of angularFire without the call to .toJSON(), and tries to save a Date object to the synced scope object. As you can see, it does not sync the Date object back to the server.

http://plnkr.co/edit/Ii6PSObgC1d8W4DFV4lF?p=preview

However, if you put back the return angular.fromJson(angular.toJson(newObj));, it syncs the Date object to the server, as JSON.stringify converts the Date object into a string behind the scenes.

@bendrucker
Copy link
Contributor

No question this is a bug. Didn't think about the fact that dates would need be stringified. Makes sense.

Would still be useful to convert the JSON serialization cycle into a manual recursive walk of the object.

@urish
Copy link
Author

urish commented Jun 17, 2014

Angular's .toJson() also does some other magic like replacing references to window with the string '$WINDOW', same for document and scope. So if we remove the call to toJson() and only do a recursive walk, this will have to be marked as a breaking change.

@katowulf
Copy link
Contributor

The 0.8 release will contain several breaking changes and this issue will probably be made obsolete (we won't be storing $ prefixes on the data or relying on angular.toJSON). It may be best to just find a workaround for now and close this out as a lot of work on a PR would be in vain.

@bendrucker
Copy link
Contributor

Would agree. Btw, guess I should have clarified that I've been referring to obj.toJSON above and not angular.toJson(obj). If an object has a method toJSON, it is called by JSON.stringify. toJSON returns a copy of the object to be stringified. It's also called recursively, so even if you stringify an object without toJSON but with a child many levels down that does use it, you'll get a copy with that child object as returned by toJSON.

Ends up being a useful formatter because it takes care of both actual serialization as well as cases where you just want to grab a clean object to do whatever you want.

@joshbowdoin
Copy link
Contributor

I'm personally sad to see the stripping of '$' prefixed properties go away in Angular (although it makes sense, and we were all warned to not use them). I've come to enjoy being able to put local variables on a model, and have them stripped off before being persisted to Firebase or my server (via $http).

With the new change, I've simply made my own JSON.stringify replacer that strips properties that have my prefix (now "A$" for me) when using the $http service.

However, this won't work with angularFire. With the change to not putting $prefixes on angularFire objects, I assume that you won't be stripping any properties off the data before saving it.

I'd like to suggest that there be a standard "local" data prefix that could still be used for this purpose, or that there is a configurable prefix that we can set for this purpose.

Use case: storing references to a parent on a child. Another use case: storing local state.

@katowulf
Copy link
Contributor

@joshbowdoin in 0.8 you'll be able to use your own toJSON methods or completely reformat the data before it is sent to the server. If you are interested in reviewing the upcoming API changes and offering feedback, shoot me an email at wulf at firebase and let's do a quick hangout! We'd love your feedback.

@urish
Copy link
Author

urish commented Jun 17, 2014

@katowulf I would also love to learn about the upcoming API changes in 0.8.0. Any timeframe for the release? I would love to start using Angular.JS 1.3.0 in my app, and currently this issue is a blocker...

Thanks!

@joshbowdoin
Copy link
Contributor

Being able to reformat the data before it is sent to the server sounds great! Just what I hoped for.

@jwngr jwngr added the bug label Jun 19, 2014
@jwngr jwngr added this to the 0.8.0 release milestone Jun 20, 2014
@katowulf
Copy link
Contributor

Resolved for the 0.8 release. I'm going to close this out.

@katowulf katowulf closed this Jul 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants