montagejs / montage

Montage is an elegant, open source HTML5 framework maintained by Montage Studio that rivals native SDKs, yet is easier to learn. It offers modular components, two-way data binding, and much more. Join us on irc.freenode.net#montage. Sign up for our beta to build Montage applications in the cloud.
http://montagestudio.com/montagejs
Other
1.51k stars 214 forks source link

Redundant call to mapping.mapRawDataToObjectProperty if mapping result isn't a Promise #2013

Open marchant opened 5 years ago

marchant commented 5 years ago

data-service.js line 1424 in _fetchObjectPropertyWithPropertyDescriptor:

-> result = mapping.mapRawDataToObjectProperty(data, object, propertyName);

This was already done a few lines up: 1415: result = mapping.mapObjectToCriteriaSourceForProperty(object, data, propertyName);

Why are we re-doing this?

Need to test removing it and make sure there are no regressions.

tejaede commented 5 years ago

The full snippet in question is the following:

//Why aren't we passing this.snapshotForObject(object) instead of copying everying in a new empty object?
Object.assign(data, this.snapshotForObject(object));
result = mapping.mapObjectToCriteriaSourceForProperty(object, data, propertyName);
if (this._isAsync(result)) {
    return result.then(function() {
        Object.assign(data, self.snapshotForObject(object));
        return mapping.mapRawDataToObjectProperty(data, object, propertyName);
    });
} else {
    //This was already done a few lines up. Why are we re-doing this?
    Object.assign(data, self.snapshotForObject(object));
    result = mapping.mapRawDataToObjectProperty(data, object, propertyName);
    if (!this._isAsync(result)) {
        result = this.nullPromise;
    }
    return result;
}

I believe the redundancy is the second call to Object.assign(data, self.snapshotForObject(object));. Aside from that, no logical path results in duplicate calls to the same method.