beeminder / road

Beebrain and Visual Graph Editor
http://graph.beeminder.com
Other
11 stars 3 forks source link

The extendo function in butil.js does bgraph option merging wrong #199

Open saranli opened 3 years ago

saranli commented 3 years ago
### Desiderata
- [ ] Brief articulation of the severity and user-visibleness
- [ ] Fix the bug
- [ ] Tweet the UVI if applicable

Default options for bgraph has inner objects to specify various suboptions. However, if an option is specified externally, inner objects are directly replaced, and if the external specification does not include all of the required parameters, bgraph will exit with errors. The solution is to go through the external options and replace provided values in inner objects as well.

Copy of the function in question for reference:

// TODO: Does not properly copy, especially for array properties. FIX
// https://github.com/beeminder/road/issues/199
// Extends a destination object with properties from a source object, optionally
// overwriting existing elements.
// @param {object}  fr Source object 
// @param {object}  to Destination object
// @param {boolean} ow Whether to overwrite existing properties of destination
function extendo(to, fr, ow) {
  let prop, hasProp
  for (prop in fr) {
    hasProp = to[prop] !== undefined
    if (hasProp && typeof fr[prop] === 'object' &&
        fr[prop] !== null  && fr[prop].nodeName === undefined ) {
      if (listy(fr[prop])) { if (ow) to[prop] = fr[prop].slice(0) }
      else to[prop] = extendo({}, fr[prop], ow)
    } else if (ow || !hasProp) {
      if (listy(fr[prop])) to[prop] = fr[prop].slice(0)
      else to[prop] = fr[prop]
    }
  }
  return to
}

Verbata: refactor, refactoring, robustness, javascript idiosyncrasies,

dreeves commented 8 months ago

GPT-4 suggests the following, for whatever this is worth:

// -----------------------------------------------------------------------------
// Extend a destination object with properties from a source object, optionally
// overwriting existing elements.
// @param {object}  fr Source object 
// @param {object}  to Destination object
// @param {boolean} ow Whether to overwrite existing properties of destination
function extendo(to, fr, ow) {
  let prop, hasProp;
  for (prop in fr) {
    if (fr.hasOwnProperty(prop)) {  // Ensure prop comes from the source object
      hasProp = to[prop] !== undefined;
      if (typeof fr[prop] === 'object' && fr[prop] !== null && 
          fr[prop].nodeName === undefined) {
        if (Array.isArray(fr[prop])) {
          to[prop] = fr[prop].map(item => {
            if (typeof item === 'object' && item !== null) {
              return extendo(Array.isArray(item) ? [] : {}, item, ow);
            }
            return item
          })
        } else {
          to[prop] = extendo({}, fr[prop], ow)
        }
      } else if (ow || !hasProp) {
        to[prop] = fr[prop]
      }
    }
  }
  return to
}