uber-node / zero-config

A zero configuration configuration loader
MIT License
116 stars 10 forks source link

Loading datacenter config contains a hidden nextTick even though all config loading is synchronous. #31

Open lxe opened 9 years ago

lxe commented 9 years ago

https://github.com/uber/zero-config/blob/master/index.js#L30-L34

        // throw error async. this allows for breaking a 
        // circular dependency between config & logger.
        process.nextTick(function () {
            config.emit('error', err);
        });

All config loading is synchrnonous. fetchConfig() returns a result, and THEN throws an error, which is undesirable and confusing. For example:

test('Non-existent dc test', function assert(t) {
  t.throws(function shouldThrow() {
    var config = zc({
      dc: '/does/not/exist',
    });
  }, /MissingDatacenterFileError/);

  t.end();
});

^ This tests will fail, and THEN the test suite will crash.

lxe commented 9 years ago

I guess I can register on error event, but this just undoes the synchronous nature of config initialization.

Raynos commented 9 years ago

As mentioned; errors need to throw async because production. There are examples on how to write the tests.

lxe commented 9 years ago

errors need to throw async because production.

I agree that If an error occurs during runtime, after the things are configured, it should of course log. If config is not initialized with all the required parameters, the process should crash.

Things that are synchronous should stay synchronous. If async operations are being performed in the background, it should do zeroConfig(..., function onConfigLoad(err, config) { ... });. Or, following the existing EE pattern: zeroConfig.on('load', function onConfigLoad(err, config) { ... })

lxe commented 9 years ago

Why do other things throw in this case? Like:

https://github.com/uber/zero-config/blob/master/index.js#L15

Raynos commented 9 years ago

Because one is a programmer error and one is an operational error.

The programmer errors are thrown and the operational errors are emitted with the standard node event emitter interface