isObject returns false given Date object

[benmccann]

https://github.com/chartjs/Chart.js/blob/master/src/helpers/helpers.core.js#L51

What is isObject supposed to return given a Date?

The method has the code:

Object.prototype.toString.call(value) === '[object Object]'

This will return false if passed a date because you'll get [object Date]

[simonbrunel]

That's the expected behavior

[benmccann]

Ok. I can't change the code in #4533 as suggested then.

Originally:

if (typeof(rawValue) === 'object') {
	if ((rawValue instanceof Date) || (rawValue instanceof moment)) {
		return rawValue;
	}

Suggested to change to:

if (helpers.isObject(rawValue)) {
	if ((rawValue instanceof Date) || (rawValue instanceof moment)) {
		return rawValue;
	}

Which won't work since helpers.isObject returns false for Date

[simonbrunel]

I suggested that actually (in core.scale.js), which is quite different:

if (helpers.isObject(rawValue)) {
    if (me.isHorizontal()) {
        if (rawValue.x !== undefined) {
            return me.getRightValue(rawValue.x);
        }
    } else if (rawValue.y !== undefined) {
        return me.getRightValue(rawValue.y);
    }
}

return rawValue;

[benmccann]

I was referring to the comment in #4533 (comment)

[simonbrunel]

And in scale.time.js:

getRightValue: function(rawValue) {
	if (helpers.isObject(rawValue) && rawValue.t !== undefined) {
		return this.getRightValue(rawValue.t);
	}
	return Chart.Scale.prototype.getRightValue.call(this, rawValue);
}

which could even be simpler:

getRightValue: function(rawValue) {
	if (rawValue && rawValue.t !== undefined) {
		return this.getRightValue(rawValue.t);
	}
	return Chart.Scale.prototype.getRightValue.call(this, rawValue);
}

[simonbrunel]

Actually, you don't even need to test if rawValue is an object, if(rawValue) should be enough and more efficient.

[benmccann]

What about the check for date types? I don't think I can use exactly the code above since it doesn't have that check?

if ((rawValue instanceof Date) || (rawValue instanceof moment)) {
	return rawValue;
}

[simonbrunel]

Why you need to test it since what you want is rawValue if it's a Date or a moment object, which is what you get with (the new) getRightValue of the base scale.

[simonbrunel]

If you really want to check Date and moment, then this should work:

getRightValue: function(rawValue) {
    if ((rawValue instanceof Date) || (rawValue instanceof moment)) {
        return rawValue;
    }
    if (rawValue && rawValue.t !== undefined) {
        return this.getRightValue(rawValue.t);
    }
    return Chart.Scale.prototype.getRightValue.call(this, rawValue);
}

[simonbrunel]

And I think you can even replace the isObject call in the base getRightValue by a simple if(rawValue) and save one function call for every data value.

[benmccann]

yeah, I guess you're right. I probably don't really need to check Date and moment. I figured there was some need to since the code was doing that originally, but as far as I can tell it's not required

[simonbrunel]

@benmccann after reading again the original implementation, I think my version is not fully correct: it should return NaN if rawValue is an object that doesn't define the x (or y) property. What do you think to fallback to NaN (instead of rawValue) and in scale.time.js explicitly check for Date and moment?

// core.scale.js
getRightValue: function(rawValue) {
    if (typeof(rawValue) === 'number' && isFinite(rawValue)) {
        return rawValue;
    }
    if (rawValue) {
        if (me.isHorizontal()) {
            if (rawValue.x !== undefined) {
                return me.getRightValue(rawValue.x);
            }
        } else if (rawValue.y !== undefined) {
            return me.getRightValue(rawValue.y);
        }
    }    
    return NaN;
},

// scale.time.js
getRightValue: function(rawValue) {
    if ((rawValue instanceof Date) || (rawValue instanceof moment)) {
        return rawValue;
    }
    if (rawValue && rawValue.t !== undefined) {
        return this.getRightValue(rawValue.t);
    }
    return Chart.Scale.prototype.getRightValue.call(this, rawValue);
}

Simpler and more predictable code (more readable as well since it becomes obvious that a "right value" is a finite number (and a Date / moment for the time scale).

[benmccann]

That causes the tests to fail. I think we're spending quite a lot of time optimizing a little section of code without fixing any issues people have actually run into or adding any functionality. Should we just call it a day on this one?

[benmccann]

To get it to work I would need to change it to:

if (typeof(rawValue) === 'string' || (typeof(rawValue) === 'number' && isFinite(rawValue))) {
	return rawValue;
}

I'll go ahead and do that

[simonbrunel]

Any idea where a string is expected as the result of getRightValue? I thought it was exclusively returning a number but if not, that means any other type of data could be valid (previous version, fallback to rawValue).

@etimberg do you know what this method is really supposed to do?