Skip to content
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

Handle the case where properties in new data can be multi-level path strings #3

Merged
merged 2 commits into from
Sep 13, 2016

Conversation

yipingliao
Copy link
Contributor


var augmentedNewData = {};
augmentedNewData[path] = newData;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make a wrapper obj, which has the passed-in path as its only key, where the value is newData. Then toss it into the helper function _convertObjToLevels, which recursively makes levels for all keys (including nested ones) with path strings (i.e., strings with "/" in them).

@yipingliao
Copy link
Contributor Author

@daguej Ping on this.

@daguej
Copy link
Contributor

daguej commented Sep 8, 2016

Do we ever see anything like:

{
  'a/b/c': 1,
  'a/b/d': 2
}

_convertObjToLevels outputs { a: { b: { d: 2 } } }

@yipingliao
Copy link
Contributor Author

yipingliao commented Sep 9, 2016

@daguej Commit pushed - data conversion function should now handle all kinds of cases where the keys inside an object are "concatenated" together at any level with "/"s.

Example input:

{
  'a/b/c': 1,
  'a/b/d': 2,
  'a/b/e/f': 3,
  'a/b/e/g': 4,
  'b': {
    'foo/bar': {
      'baz': 10
    }
  },
  's/u/n': {
    'm/o/n': {
      's': 9,
      't': 8,
      'a': 7,
      'r': 6
    }
  },
  'one/two': {
    'three/four': {
      'five': 'A'
    }
  },
  'one/two/three': {
    'four': {
      'six': 'B'
    }
  },
  'one': {
    'two/three/four/seven': {
      'eight': {
        'nine/ten': 'C',
        'eleven': {}
      }
    }
  }
}

Corresponding output:

{
  "a": {
    "b": {
      "c": 1,
      "d": 2,
      "e": {
        "f": 3,
        "g": 4
      }
    }
  },
  "b": {
    "foo": {
      "bar": {
        "baz": 10
      }
    }
  },
  "s": {
    "u": {
      "n": {
        "m": {
          "o": {
            "n": {
              "s": 9,
              "t": 8,
              "a": 7,
              "r": 6
            }
          }
        }
      }
    }
  },
  "one": {
    "two": {
      "three": {
        "four": {
          "five": "A",
          "six": "B",
          "seven": {
            "eight": {
              "nine": {
                "ten": "C"
              }
              "eleven": {}
            }
          }
        }
      }
    }
  }
}

@yipingliao yipingliao force-pushed the bug/handle-alt-fb-new-data-format branch from ccf083b to 713cfae Compare September 9, 2016 18:17
return inputObj;
}
return inputObj;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray spaces

@yipingliao yipingliao force-pushed the bug/handle-alt-fb-new-data-format branch from 713cfae to 97ab280 Compare September 9, 2016 20:56
@yipingliao
Copy link
Contributor Author

Fixed spaces. :)

// If the current raw object is a scalar or just the empty object, navigate to
// where this raw object should live by walking down the path in resultObj, and
// put this raw object there.
if ((typeof currentRawObj !== 'object') || (!Object.keys(currentRawObj).length)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential performance optimization: we'll end up enumerating all of the keys of the object twice -- here and at 492.

Since firebase doesn't support empty objects, can we avoid the Object.keys(currentRawObj).length altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I forgot that Firebase does not support empty objects, so there is no need for this check. Will push changes shortly.

@yipingliao yipingliao force-pushed the bug/handle-alt-fb-new-data-format branch from 97ab280 to 28b2045 Compare September 9, 2016 21:47
@yipingliao
Copy link
Contributor Author

Ping on this again. :) @daguej

@daguej daguej merged commit e25cd64 into master Sep 13, 2016
@daguej daguej deleted the bug/handle-alt-fb-new-data-format branch September 13, 2016 17:22
@daguej
Copy link
Contributor

daguej commented Sep 13, 2016

Released as v2.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants