@lorenwest This PR seems consistent with some other special cases we handle and includes test coverage. I recommend merging it.

A competing PR was merged, but there are parts I like better here. I like that this returns Uint8Array and not a buffer, and I think the test coverage here was more clearly organized. I would like to get the test coverage contributed for both cases merged like this:

'use strict';

const requireUncached = require('./_utils/requireUncached');
require('../parser');

'use strict';

const vows = require('vows');
const assert = require('assert');

vows.describe('Tests for parsing binary data')
  .addBatch({
    'Using the YAML parser - Uint8Array': {
      topic: function() {
        process.env.NODE_CONFIG_DIR = __dirname + '/22-binary';
        return requireUncached(__dirname + '/../lib/config');
      },
      'A binary value should be immutable': function(config) {
        assert.throws(() => {
          const auth = config.get('auth');
          auth.secret = new Uint8Array([ 1, 2, 3 ]);
        }, /Can not update runtime configuration/)
      },

      'reading !!binary returns a real Uint8Array': function(CONFIG) {
        assert.deepStrictEqual(CONFIG.get('auth'), {
					secret: new Uint8Array([0x10, 0x11, 0x12, 0x13, 0x14, 0x15])
				});
      },
    }
  })
  .export(module);

I don’t think exposing mutable data was meant to be part of the Config design and API.

In fact I’m not happy with the current Buffer behavior referenced at the top of this function. That seems wrong. It should at least be a copy operation.

I believe if you’ve disabled immutability this code doesn’t get called at all, so we are just talking about the default behavior of disallowing modification after initialization.

Mutability make the config into global shared state. There’s a place for that, and if you can overlay a single writer api over the top of node-config you can do a lot of cool stuff. As long as every function you call can modify that state it’s chaos for local reasoning. Not to mention testing. You might as well just use global.*.

Global shared state doesn’t scale past a few ambitious coworkers trying to “get things done” and deal with consequences “later”. If you’ve picked up node-config you probably trying to avoid that

I think I’d be open to snapshotting. And a new TypedArray doesn’t have to be built on a Buffer. It can be created on an Array built from the source buffer. Though how are you even getting a buffer into config in the first place?

Ancient versions of the library did support mutability. Making the config structure immutable was something I advocated and Loren agreed, and keeping the config state immutable has been a priority since.

Good, then we are all three in accord (I've had this conversation before with Loren as well).

I think there is a hole here, that I would like to make smaller or close if we can. But I think this solution is going left when we want to go right.

An part of that is knowing more about the use case. I'm still in the 'why would you encounter this?' bubble and that's not a good place to make a counterproposal from.

I'm still in the 'why would you encounter this?' bubble and that's not a good place to make a counterproposal from.

There's a test case to illustrate. It seems it can happen when you store binary objects in a YAML config file.

It looks like there's alignment about merging this, but now there are conflicts that need to be resolved.

I don't know if I'd say there's alignment but I am currently bumping into this issue trying to clean up makeImmutable, which is only guarding against converting Buffers at the top level but not when looking at the children.

But again, why do we have features that were supported to add a mutable object into the middle of immutable data?

So here's what I propose.

I'm going to take on extracting makeImmutable to Util for 4.4 or a 4.5 build.

I'm going to pull in some of @Maia-Everett's commits to retain attribution, then I'm going to move the code to lib/util.js, rearrange some of the guts of makeImmutable and cloneDeep so they don't look so much like individual snowflakes. I think I prefer the shape of cloneDeep to makeImmutable. I might need to extract some bits to make that happen.

Then I'm going to deprecate that config.util.makeImmutable(obj, propertyName, value) feature because what is that even for? It's making a method that is getting called a massive number of times on large config bodies that much slower and hard to read. And we don't even use it, and haven't for most of the life of this project. GTFO.

But first I have too many branches open at the same time and I need to land some of those before I can tackle this.

There's already a change earmarked for 4.4 that affects how makeImmutable works. Might as well make it two to reduce the number of fire drills users have to experience.

I'm having second thoughts on this one. Without these changes, it seems that you can get what you want by using

Buffer.from(config.auth.secret);