added File object conditional check to _processKeys

[bitwurx]

No description provided.

[lightswitch05]

👍

[lightswitch05]

@domchristie any comments on getting this merged?

[domchristie]

@bitwurx @lightswitch05 sorry for the delay in responding. It has been a while since I looked at this part of the code, and I’m trying to work out what its real purpose is!

I think what we want to do is return obj whenever obj cannot be processed i.e. when obj is not a plain object or an array. I wonder if we can make this code more robust by replacing all those _isTYPE checks with something like:

if(!(_isArray(obj) || _isPlainObject(obj))) {
  return obj;
}

and

var _isPlainObject = function(obj) {
  return _isObject(obj) && obj.constructor === Object;
};

What do you reckon?

[lightswitch05]

It would clean up the code, but we often use humps on custom objects (decamelize our JavaScript models for a snake_case API). I think _isPlainObject would break that behavior.

[domchristie]

Interesting.

Well the issue is that if we check for File objects, then we should also check all other JavaScript types (e.g. https://developer.mozilla.org/en-US/docs/Web/API), which could get a little silly! So it’s not just a case of cleaning up the code, but rather making it work in all cases, now and in the future.

[lightswitch05]

You have some great points. After looking into it more with @bitwurx , this pull request (and the other type checking already in the library) is really a work around for the behavior of returning an entirely new object (line 30). I think having File and Date objects would be fine as long as they were preserved instead of turned into a generic Object. This would preserve backwards compatibility with the existing type checks (Date, Regex) - and allow support for all the other JavaScript types.

[domchristie]

Yeh, the reason for creating a new object was to effectively clone the given object, though I suppose you’re saying that this may not be necessary? (again, apologies: I wrote this a while ago, and so cannot be sure if there was another reason for this!).

We may run into issues if we alter the given object, particularly if it’s of a native type or if the property is read-only.

In terms of preserving some objects, would you be able to show me what you have in mind (e.g. with code/pseudocode)

Thanks!

[lightswitch05]

I think this is the current pseudo-code for _processKeys

Is the given item a special type? (Boolean, int, Date, regex, file)
    return the special type un-modified
Is the given item an array?
    Make a new generic array to copy into
    For each item in the given array
        call _processKeys and add return to the new generic array
By now we've tested against special types or arrays, it must be an object then!
    Make a new generic object to copy into
    For each own property in my given object
        call _processKeys and add return to the new generic object using a humped key

Here is what I have in mind for preserving the object type. It wouldn't work as-is, but will hopefully explain my logic:

   if(_isArray(obj)) {
      for(l=obj.length; i<l; i++) {
        // no need to duplicate the array
        obj[i] = (_processKeys(convert, obj[i], options));
      }
    }
    else { // will still need to do some primitive type checking
      for(var key in obj) {
          try {
              var convertedKey = convert(key, options);
              obj[convertedKey] = _processKeys(convert, obj[key], options);
              delete obj[key];
          } catch(err) {
              // don't care - must a special object ignore and continue
          }
      }
    }

Here is a quick mockup that uses Date, RegExp, custom objects, and plain objects: https://repl.it/BZSY/2

[domchristie]

Excellent!

I’m slightly concerned about the mutation, but happy to go for this direction in v2 👍

[lightswitch05]

I've been thinking about this more:

• Object mutation is scary
• Backwards compatibility might not be maintained
• Doesn't fix circular references: I've got a special object, in my specific case its kendo.data.ObservableObject. This wonderful 😑 object ends up making circular references to itself - so obviously stepping through the properties isn't a solution.

I think the best way forward is to define a strategy for humps to use:

1. Clone - this is the exact current behavior

2. Mutate - What we described above

3. Stringify - My object with circular references defines a toJSON function. I believe that the majority of people using this tool are probably using it in an API layer right before or right after converting an object between JSON. Right now I'm getting around the circular reference by calling JSON.stringify() and then JSON.parse() immediately after:

   var terribleObject = new kendo.data.ObservableObject({description: 'This is an example'});
   terribleObject = JSON.stringify(terribleObject);
   terribleObject = JSON.parse(terribleObject);
   terribleObject = humps.decamelize(terribleObject);
   API.send(terribleObject);
   

Stringify would be an extremely safe method to normalize the objects before trying to modify them - but it also wouldn't preserve types or preserve currently functionality. Having a way to define hump's strategy would keep everyone happy and current functionality could be retained by default.

[domchristie]

@lightswitch05 Thanks for putting some though into this, and apologies again for getting back sooner.

I think the best way forward is to define a strategy for humps to use

Do you mean as a configuration?

I’m in two minds about stringifying and parsing. I like it in that it’s an elegant way to normalise input (especially native types), but as with mutation, it feels a bit destructive.

[lightswitch05]

Yes, as a configuration. Each conversation method has its own strengths and weaknesses. The only way to support all object types is to support different conversion stratagies. It would be great if you could think of a way to auto detect which strategy is going to work best depending on the object type- but I don't know how that would work.

[MarkMurphy]

How about this:

var _processKeys = function(convert, obj, options) {
  var output,
      i = 0,
      l = 0;

  if (_isArray(obj)) {
    output = options.strategy === 'mutate' ? obj : [];

    for (l = obj.length; i < l; i++) {
      output[i] = _processKeys(convert, obj[i], options);
    }

    return output;
  }

  if (_isObject(obj)) {
    output = options.strategy === 'mutate' ? obj : {};

    for (var key in obj) {
      if (obj.hasOwnProperty(key)) {
        var convertedKey = convert(key, options)
        output[convertedKey] = _processKeys(convert, obj[key], options);

        if (options.strategy === 'mutate') {
          delete obj[key]
        }
      }
    }

    return output;
  }

  return obj;
}

var _isObject = function(obj) {
  return toString.call(obj) === '[object Object]';
};

I think this addresses everyone's concerns and is backward compatible

[Rush]

To maximize performance mutation should be implemented in a separate function.

Preferably it should have interface like so:
humps.camelizeKeysInPlace(object)

edit: I created xcase https://github.com/encharm/xcase which has xcase.camelizeKeys(obj, {inPlace: true})

[byzg]

Hi guys, unfortunately the operation with the files still does not work. Maybe now it makes sense to add support for files, and then think about what to do with all the other JavaScript types