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

Any model with a "." in the key name will cause observe.js _set() to throw and error #257

Closed
colinexl opened this issue Jan 25, 2013 · 4 comments · Fixed by #318
Closed
Labels
Milestone

Comments

@colinexl
Copy link

If our Rails server returns the following JSON:

[{
  "1.0": "Foo",
  "Bar": "Zaz"
}]

This will cause attrParts() to split the period in "1.0" and will try to call __get() on "1" when "1" is not a valid object.

@colinexl
Copy link
Author

colinexl commented Feb 7, 2013

Here's a JSFiddle example: http://jsfiddle.net/7YvyV/

@daffl
Copy link
Contributor

daffl commented Feb 7, 2013

Thanks. Marked for the next 1.1.5 release. I'm thinking one solution would be to not use dot separated accessors when you pass an object to .attr().

The problem would be that you couldn't do .attr('1.0') because it would try to get [1][0].

@colinexl
Copy link
Author

colinexl commented Feb 7, 2013

Thanks for the response. We implemented a really hacky fix for this on our local branch to get this to work, diff below:

diff --git a/observe/observe.js b/observe/observe.js
index 73629d7..03e07f3 100644
--- a/observe/observe.js
+++ b/observe/observe.js

@@ -604,13 +604,23 @@ steal('can/util','can/construct', function(can) {
                // `attr` - Is a string of properties or an array  of property values.
                // `value` - The raw value to set.
                _set: function( attr, value ) {
+                       // TODO: this is a hacky fix to undo what attrParts did
+                       var nesting = attr.indexOf(".") > -1;
+
                        // Convert `attr` to attr parts (if it isn't already).
-                       var parts = attrParts(attr),
+                       var parts = attrParts(attr);
+
+                       // TODO: this is a hacky fix to undo what attrParts did
+                       if(nesting && this.attr(parts[0]) === undefined) {
+                               parts = [parts.join('.')];
+                       }
+
                                // The immediate prop we are setting.
-                               prop = parts.shift(),
+                       var     prop = parts.shift(),
                                // The current value.
                                current = this.__get(prop);

+
                        // If we have an `object` and remaining parts.
                        if ( canMakeObserve(current) && parts.length ) {
                                // That `object` should set it (this might need to call attr).

This was meant as a quick fix in order to get our app to run but by new means is this a long term solution. Looking forward to a real fix in 1.1.5.

-Colin Zhu

@daffl
Copy link
Contributor

daffl commented Feb 21, 2013

I think we'll have to bump this to 1.2 because any fix would entail a change it how it works. My suggestion:

var ob = new can.Observe();

ob.attr('a.b', 'test');
ob.attr() // -> { a: { b: 'test' } }

ob.attr({
    'a.b': 'test'
});

ob.attr() // -> { 'a.b': 'test' }

ob.attr('a.b')
// will look for 'a.b' first
// and then for ob.attr('a').attr('b')

That way it wouldn't break too much and still stay mostly backwards compatible. We still can't introduce it in a patch version though.

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

Successfully merging a pull request may close this issue.

2 participants