Is the relative file path supported?

[snowdream]

nconf.use('file', { file: '../config.json' });

[kreegr]

Looks like it is supported. However, there's no error thrown when the path fails to resolve - so check your path. For me, the path resolves relative to where the process (node) is run from. I used something like:

file:path.resolve(__dirname, '..rel/path/conf.json)

[avimar]

../ seems to work just fine.

[davidlivingrooms]

Sorry to post on such an old thread but this issue caused me some headache...

The fact that the import fails silently can cause some tricky to find bugs. In my case my port was pulled as undefined and passed to express which randomly assigns a port if none is passed in. This took me an hour to track down :(

I think there could be better messaging when failing to import a file. Maybe it should throw an exception or at least console.warn() if it can't find your file? I don't mind submitting a PR if you would like.

[mhamann]

@davidlivingrooms would love to have you submit a PR around this! I agree that it would be great to ensure that nconf produces a warning when a file cannot be found.

All we ask is that PRs include pertinent tests :-)

[davidlivingrooms]

@mhamann Sounds good, looking into it now. Just FYI this is my first PR so please let me know if I miss anything and i'll fix it 😄 .

As for the solution: It sounds like we want to preserve the behavior of creating the store for them if the file is not found. In that case my initial thought was just to add a console.warn in loadSync and load.

file.js

File.prototype.loadSync = function () {
  if (!existsSync(this.file)) {
    console.warn('Failed to find your configuration file: ' + this.file) // Now you know your file failed to load!
    this.store = {};
    return this.store;
  }

  // rest of func
};

File.prototype.load = function (callback) {
  exists(self.file, function (exists) {
    if (!exists) {
      console.warn('Failed to find your configuration file: ' + this.file) // Now you know your file failed to load!      
      return callback(null, {});
    }

    // rest of func 
  });
};

Does this sound like the right solution to you? Initially I thought we should throw an error but then we would have to create their store and return it in the catch block which felt a little weird to me...let me know what you think.

Also, I haven't written a test that asserts console output before and wasn't sure how to approach it. I found this and it seemed okay for a test. The gist is that we override console.log during the tests and then reassign after the test has run. How do you feel about this approach? Let me know if there is a different way you would prefer to test this. Thanks!

[mhamann]

The logic you show looks pretty good.

However, it's actually a bit more complicated than that. We don't want to include console.* statements in our code, because they are I/O operations that are also blocking, which could bottleneck production code depending on how it's used.

nconf doesn't really do any logging on its own right now, though I think there are some places where that would be pretty useful (e.g. if you wanted to trace how your config is getting loaded, etc).

We would want to give the consuming code control over logging, so that would mean enabling a developer to pass a "logging" instance to an nconf provider that it could then use to produce messages.

We would need to define an expected contract that loggers should follow, such as having methods like: error, warn, info, debug, trace, etc

If no logger is provided, log statements are basically a no-op. (@indexzero, let me know if you have opinions on this topic in general).

Once that's in place, then it's easy to spit out warnings from the code you showed above.

[avimar]

@mhamann if you're really worried about i/o operations, then we should skip the call to exists and just use the var fileData = fs.readFileSync(this.file, 'utf8'); in https://github.com/indexzero/nconf/blob/391665cc38412ce47c31c9f0eea8da6e76fe2663/lib/nconf/stores/file.js#L162 ... and add a try/catch for err.code === 'ENOENT'

That's what the docs recommend, too:

https://nodejs.org/api/fs.html#fs_fs_exists_path_callback

Using fs.exists() to check for the existence of a file before calling fs.open(), fs.readFile() or fs.writeFile() is not recommended. Doing so introduces a race condition, since other processes may change the file's state between the two calls. Instead, user code should open/read/write the file directly and handle the error raised if the file does not exist.

[davidlivingrooms]

Throwing out another potential option. If we are concerned with logging to stdout in production we could use the debug module. This would allow devs to opt-in to our logging without breaking existing users. This is how express approaches extra logging.