Improve deepFreeze function a bit

[romanenko]

Check for null properties and freeze functions as well

[winkler1]

Hey, thanks for the PR!
Silly question - why freeze functions? With the JSON serialization/cloning, seems like they'll be stripped out-

[aaronjensen]

Also, I believe freezing functions requires some more care as you cannot freeze caller, callee, or arguments in strict mode.

Not freezing null is a must though, it'll throw in browsers/phantomjs if you don't.

It's unfortunate that https://github.com/substack/deep-freeze and https://github.com/jsdf/deep-freeze are buggy. If they weren't, we wouldn't need to roll our own

Maybe it'd be worth pulling out a common implementation for just the freeze that updeep and icedam can depend on?

[aaronjensen]

As an aside, @winkler1, I noticed you used Object.getOwnPropertyNames instead of Object.keys. The former gets non-enumerable properties and the latter does not. In your experience, is there need for the former? I'm wondering if I should switch mine... the types of things updeep deals w/ should be plain objects/arrays, so I don't feel like I need to worry about read-only properties, but that could be naive

[winkler1]

@aaronjensen Adding the null check and an explicit test, thanks. The Redux devtools make a point of saying to use serializable data structures for its time travel abilities. Will beef up the tests for null and function.

A common deepFreeze sounds good if you want. My only concern would be that the import/require can be guarded by a if dev block:

var deepFreeze, shallowEqual;

if (process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') {
  deepFreeze = require('commonDeepFreeze');
  shallowEqual = require('react-pure-render/shallowEqual');
}

Really want to ensure that in production this is a 3-line no-op, no dependencies...

[winkler1]

@aaronjensen re:enumerable - now that you mention it... non-enumerable props are stripped by JSON.stringify cloning, so Object.keys would work just as well.

I'll add a test to make this explicit:

  it('Removes non-enumerable properties', function () {
    var input = {};
    Object.defineProperties(input, {
      a: {enumerable: true, value: 'a'},
      b: {enumerable: false, value: 'b'}
    });
    var output = freeze(input);
    expect(Object.keys(output)).toEqual(['a']);
  });

[aaronjensen]

👍

wrt to dev vs prod, a couple things:

You might consider looking for NODE_ENV !== 'production' as the sign to include things as this matches react: https://github.com/facebook/react/blob/d16481d0e7d23771c5d94e5a0eeaf64e02f07979/packages/react/react.js#L9

Also, for this lib it'd remove a lot of conditionals and may make the code a bit easier if you did something like:

if (process.env.NODE_ENV === 'production') {
  module.exports = function makeFreezer() {
    function(obj) { return obj; }
  }
} else {
  // existing code
  module.exports = function...
}

[aaronjensen]

Actually, I went to do this on updeep and found that everything in freeze.js was already dead code eliminated by uglify--it's pretty smart.