Config for appRoot and port is never read from config/default.yml

[haimich]

First of all: kudos to Bucky, I really like the library and the documentation makes it relatively easy to get started. With BuckyServer version 0.2.0 though I noticed a problem with the processing of the config file under config/default.yml: in the function loadApp a config object is passed by as second argument for usage with the load() call. The problem is that the APP_ROOT and the port are also read from this config object, which is wrong - instead they should be read from the config object defined in line 6, which unfortunately has the same name. The result of this is that setting the app root and/or the port in config.yml won't affect Bucky in any way!

[zackbloom]

I could be misreading it, but the intent (and how I read it now), is that loadConfig allows you to either specify a config module, or fallsback to using the config object. If you don't have a config module specified, loadConfig should provide loadApp with the same config object (wrapped to be async). Did you actually run into this problem in practice?

[zackbloom]

I agree that using the same variable name is poor form, I'll take care of that.

[zackbloom]

a27dbafb4be4fd6aaf409ef7466652c55848d89d

[haimich]

Wow that was fast :smile: Actually what I ment was something slightly different, so I'll try to describe it in another way: I downloaded BuckyServer, typed npm install (I didn't want it to be globally installed) and started the server with node start.js. The output was

Binding Routes at /
Server listening on port 5000 in development mode

although the default.yml has port 5999 and "bucky" as appRoot configured. So when debugging this problem I noticed that in loadApp() you are setting APP_ROOT and port like this:

APP_ROOT = process.env.APP_ROOT ? config.server?.appRoot ? ''

The problem here is that the config object passed to loadApp() seems to have a different syntax then the one loaded with require 'config'. So one fix would be to use the correct syntax with

config.get('server.appRoot').get()

Another possibility would be to rename the config argument as you did but don't use this variable for reading the appRoot/port but the one defined in line 6.

I also tried your recent commit without noticing any difference.

[zackbloom]

Ahhh, you're right! Standby.

[zackbloom]

Fixed by 0c28bf9c6b2b9e73d5451f6b6d68414c30517bc6

Let me know if that works for you, and I'll package it as an npm release. Thanks!

[haimich]

Works like a charm, thanks for the ultra fast response!

[zackbloom]

Published as 0.3.0