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

Comments

Projects
None yet
2 participants
@colinexl

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

This comment has been minimized.

Show comment
Hide comment
@colinexl

colinexl Feb 7, 2013

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

colinexl commented Feb 7, 2013

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

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Feb 7, 2013

Contributor

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].

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

This comment has been minimized.

Show comment
Hide comment
@colinexl

colinexl 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

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Feb 21, 2013

Contributor

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.

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