913259 - Add helpers for safely checking and setting deep object properties

Status: RESOLVED WONTFIX

(Firefox for Metro Graveyard :: General, defect)

Description

[Sam Foster [:sfoster] (he/him)]

We've got a lot of (if this && this.that && this.that.theOther) guards, and by the looks of the console we need lots more - especially as bindings come unbound and other ephemeral objects cease to be before their properties are accessed.

I've used a getObjectValue(obj, "some.key") and setObjectValue(obj, "some.key", "value") kind of utility on past projects. Something similar on Util might be useful.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Add get/set helpers to avoid null-checks in nested object properties

2 Util methods + tests for getting and setting potentially-deep properties using a dot-path like "some.nested.property" for lookup. 

I'm still open to opinions on the actual utility of this, or if it will simply mask errors we would prefer to know about.

Comment 2

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 801060 [details] [diff] [review]
Add get/set helpers to avoid null-checks in nested object properties

Review of attachment 801060 [details] [diff] [review]:
-----------------------------------------------------------------

I think these methods are a good addition to util.

::: browser/metro/modules/ContentUtil.jsm
@@ +104,5 @@
> +  },
> +
> +  setValueInObject: function setValue(obj, key, value, shouldCreateAncestors) {
> +    // set a value where the key might be a dot-path
> +    // e.g. setValue(obj, 'foo.bar', "Bar") === obj.foo.bar = "Bar";

nit: In most places I've seen comments describing a method's functionality to be place before the method definition.

@@ +115,5 @@
> +    let lastKey = pathParts.pop();
> +    while ((key = pathParts.shift())) {
> +      if (!(key in obj)) {
> +        if (shouldCreateAncestors) {
> +          obj[key] = {};

This can cause some unexpected side effects but can be useful for creating mock object in tests for example. Glad that shouldCreateAncestors is not the default.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Yeah creating the ancestors in setValueInObject may just be too evil a potential source of bugs here. I've used it in other projects where it was pretty safe to assume that all ancestor objects were plain old Objects. In this context I can imagine later simple null checks returning true allowing things to fail mysteriously as a values was a object rather than some nsI*, Map or whatever. I think I'll axe that option. Besides, mbrubeck is about to tell me its bad boolean-trap smell anyhow :)

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm still digesting the use cases. I worry about the performance hit of splitting the string when the code happens to fall in a tight loop, indirectly as functions calling functions can hide things. My OCD want to name the getter a bit differently to convey the context a bit more: maybeGetValueFromObject is a bit long, but the "maybe" conveys why we even want to call this helper. If we were sure, we'd just call it directly.

I was also trying to think of other ways out of this situation. I see several place where we do:

  let someItem = this._getSomeItem();
  if (someItem && someItem.fooBar)
    sendPeopleToMars();

but since the code was refactored a few times, this._getSomeItem now throws instead of returning null and we should fixup the if condition:

  let someItem = this._getSomeItem();
  if (someItem.fooBar)
    sendPeopleToMars();

(In reply to Sam Foster [:sfoster] from comment #3)
> I think I'll axe that option. Besides, mbrubeck is about to
> tell me its bad boolean-trap smell anyhow :)

Yes!

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Add get/set helpers to avoid null-checks in nested object properties (w/o shouldCreateAncestors)

Ok, I ditched the shouldCreateAncestors option. 

Just yesterday I worked on a patch where I needed 'foo' in bar checks for a few properties. But I hesitated to add a helper like this there out of concern that it was unnecessary abstraction that might obscure what's going on. 

After getting rid of the createAncestors option, the setter doesn't really add much value over an simple assignment. The getter, I find myself needing a 'has' check more than necessarily needing the value of that property. Maybe something like hasProperties('foo', 'bar', 'foo.fooish.fool') or something. And yes, as Tim pointed out, object property names can include periods, so for a general purpose utility, it should probably support 'foo.bar'.baz.brum. Bleagh. My appetite for this helper is suddenly diminished :) 

So, I'm inclined to leave this patch on here as a possibility to be revisited as and when we feel there's a real problem that needs solving.

Comment 6

[Sam Foster [:sfoster] (he/him)]

I'm not working on this currently