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

Unexpected behaviour while using a Map #667

Closed
RpprRoger opened this Issue Jan 15, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@RpprRoger
Contributor

RpprRoger commented Jan 15, 2014

I believe I've encountered a bug/unexpected behaviour while trying to access the property of a Model. Say we have a model/map that looks like:

var test = new can.Model({
    'my.item': false
});

and we access it by

var result = test.attr('my.item');

then result is === undefined, instead of false.

I have a small change in mind which will resolve this issue, by making a change to this check https://github.com/bitovi/canjs/blob/master/map/map.js#L647 .

var value;
if( typeof attr === 'string' && !!~attr.indexOf('.') ) {
    value = this.__get(attr);
    if( value !== undefined ) {
        return value;
    }
}

I have this ready for pull request, unless of course this is expected :)

I have reproduced it in this fiddle http://jsfiddle.net/qd73h/1/.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 15, 2014

Contributor

This is expected behavior. You read that property like: "my.item".

Sent from my iPhone

On Jan 15, 2014, at 10:19 AM, Robert Preus-MacLaren notifications@github.com wrote:

I believe I've encountered a bug/unexpected behaviour while trying to access the property of a Model. Say we have a model/map that looks like:

var test = new can.Model({
'my.item': false
});
and we access it by

var result = test.attr('my.item');
then result is === undefined, instead of false.

I have a small change in mind which will resolve this issue, by making a change to this check https://github.com/bitovi/canjs/blob/master/map/map.js#L647 .

var value;
if( typeof attr === 'string' && !!~attr.indexOf('.') ) {
value = this.__get(attr);
if( value !== undefined ) {
return value;
}
}
I have this ready for pull request, unless of course this is expected :)

I have reproduced it in this fiddle http://jsfiddle.net/qd73h/1/.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 15, 2014

This is expected behavior. You read that property like: "my.item".

Sent from my iPhone

On Jan 15, 2014, at 10:19 AM, Robert Preus-MacLaren notifications@github.com wrote:

I believe I've encountered a bug/unexpected behaviour while trying to access the property of a Model. Say we have a model/map that looks like:

var test = new can.Model({
'my.item': false
});
and we access it by

var result = test.attr('my.item');
then result is === undefined, instead of false.

I have a small change in mind which will resolve this issue, by making a change to this check https://github.com/bitovi/canjs/blob/master/map/map.js#L647 .

var value;
if( typeof attr === 'string' && !!~attr.indexOf('.') ) {
value = this.__get(attr);
if( value !== undefined ) {
return value;
}
}
I have this ready for pull request, unless of course this is expected :)

I have reproduced it in this fiddle http://jsfiddle.net/qd73h/1/.


Reply to this email directly or view it on GitHub.

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Jan 15, 2014

Contributor

I think this might be broken only when the value you are trying to retrieve is falsey.

http://jsfiddle.net/thecountofzero/tt7uL/

Contributor

thecountofzero commented Jan 15, 2014

I think this might be broken only when the value you are trying to retrieve is falsey.

http://jsfiddle.net/thecountofzero/tt7uL/

@RpprRoger

This comment has been minimized.

Show comment
Hide comment
@RpprRoger

RpprRoger Jan 16, 2014

Contributor

Precisely @thecountofzero , there appears to be a check for "." and then to return that element if it is found. But the check is only if it's truthy, this is very odd behaviour, shouldn't it be one or the other? @justinbmeyer
https://github.com/bitovi/canjs/blob/master/map/map.js#L648
https://github.com/bitovi/canjs/blob/master/map/map.js#L650

The Attr functionality currently works like this

var result, test = new can.Model({
    'my.enable': false,
    'my.item': true,
    'my.count': 0,
    'my.newCount': 1
});

result = test.attr('my.enable'); // result === undefined
result = test.attr('my.item'); // result === true
result = test.attr('my.count'); // result === undefined
result = test.attr('my.newCount'); // result === 1

That doesn't feel right to me.

Contributor

RpprRoger commented Jan 16, 2014

Precisely @thecountofzero , there appears to be a check for "." and then to return that element if it is found. But the check is only if it's truthy, this is very odd behaviour, shouldn't it be one or the other? @justinbmeyer
https://github.com/bitovi/canjs/blob/master/map/map.js#L648
https://github.com/bitovi/canjs/blob/master/map/map.js#L650

The Attr functionality currently works like this

var result, test = new can.Model({
    'my.enable': false,
    'my.item': true,
    'my.count': 0,
    'my.newCount': 1
});

result = test.attr('my.enable'); // result === undefined
result = test.attr('my.item'); // result === true
result = test.attr('my.count'); // result === undefined
result = test.attr('my.newCount'); // result === 1

That doesn't feel right to me.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

Use the "."

Sent from my iPhone

On Jan 16, 2014, at 4:04 AM, Robert Preus-MacLaren notifications@github.com wrote:

Precisely @thecountofzero , there appears to be a check for "." and then to return that element if it is found. But the check is only if it's truthy, this is very odd behaviour, shouldn't it be one or the other? @justinbmeyer

The Attr functionality currently works like this

var result, test = new can.Model({
'my.enable': false,
'my.item': true,
'my.count': 0,
'my.newCount': 1
});

result = test.attr('my.enable'); // result === undefined
result = test.attr('my.item'); // result === true
result = test.attr('my.count'); // result === undefined
result = test.attr('my.newCount'); // result === 1
That doesn't feel right to me.


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Jan 16, 2014

Use the "."

Sent from my iPhone

On Jan 16, 2014, at 4:04 AM, Robert Preus-MacLaren notifications@github.com wrote:

Precisely @thecountofzero , there appears to be a check for "." and then to return that element if it is found. But the check is only if it's truthy, this is very odd behaviour, shouldn't it be one or the other? @justinbmeyer

The Attr functionality currently works like this

var result, test = new can.Model({
'my.enable': false,
'my.item': true,
'my.count': 0,
'my.newCount': 1
});

result = test.attr('my.enable'); // result === undefined
result = test.attr('my.item'); // result === true
result = test.attr('my.count'); // result === undefined
result = test.attr('my.newCount'); // result === 1
That doesn't feel right to me.


Reply to this email directly or view it on GitHub.

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Jan 16, 2014

Contributor

Using the "." works, but if the value of the attribute you are retrieving is a falsey value, it returns "undefined".
If the value of the attribute you are retrieving is not a falsey value, it returns the correct value.

http://jsfiddle.net/thecountofzero/tt7uL/

Contributor

thecountofzero commented Jan 16, 2014

Using the "." works, but if the value of the attribute you are retrieving is a falsey value, it returns "undefined".
If the value of the attribute you are retrieving is not a falsey value, it returns the correct value.

http://jsfiddle.net/thecountofzero/tt7uL/

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

Getting a falsey value should probably not work without the "".

Contributor

justinbmeyer commented Jan 16, 2014

Getting a falsey value should probably not work without the "".

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 16, 2014

Contributor

Really, if you can escape the . then something like test.attr('my.item'); should only work for { my: { item: value } } and never for { 'my.item': value } (which should be test.attr('my\.item');).

Contributor

daffl commented Jan 16, 2014

Really, if you can escape the . then something like test.attr('my.item'); should only work for { my: { item: value } } and never for { 'my.item': value } (which should be test.attr('my\.item');).

@daffl daffl reopened this Jan 16, 2014

@thecountofzero

This comment has been minimized.

Show comment
Hide comment
@thecountofzero

thecountofzero Jan 16, 2014

Contributor

Getting a falsey value fails with the ""

Contributor

thecountofzero commented Jan 16, 2014

Getting a falsey value fails with the ""

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 16, 2014

Contributor

That's why I reopened. It's really inconsistent. And broken for falsy values.

Contributor

daffl commented Jan 16, 2014

That's why I reopened. It's really inconsistent. And broken for falsy values.

@RpprRoger

This comment has been minimized.

Show comment
Hide comment
@RpprRoger

RpprRoger Jan 16, 2014

Contributor

Out of interest, isn't it true that in javascript

"\.there's a dot\." === ".there's a dot."

So at the point we do the check the backslash is irrelevant? https://github.com/bitovi/canjs/blob/master/map/map.js#L648..
Unless I'm missing something when we check the index of "."

Contributor

RpprRoger commented Jan 16, 2014

Out of interest, isn't it true that in javascript

"\.there's a dot\." === ".there's a dot."

So at the point we do the check the backslash is irrelevant? https://github.com/bitovi/canjs/blob/master/map/map.js#L648..
Unless I'm missing something when we check the index of "."

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 23, 2014

Contributor

Fixed with #671.

Contributor

andykant commented Jan 23, 2014

Fixed with #671.

@andykant andykant closed this Jan 23, 2014

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