Environment config not loaded

[jaumard]

When environment is set to production, trails doesn't override default config by config/env/production.js file.

For example trailpack-repl is not disable, I can see debug logs and I put some config for trailpack-gulp to build assets in production mode but it take the dev tasks.

The command to test the issue

$ NODE_ENV=production node server.js

Maybe something wrong around here https://github.com/trailsjs/trailpack/blob/master/index.js#L43 methods return merge config but it was not used ?

[robertrossmann]

The problem is right here - by the time this line is called, the environment-specific configuration should be merged into the main this.config object, but it is not.

Assuming the configuration merging should behave identically to how Sails merges its environment-specific configuration options, a good place to do such merge right after this line.

Seems like an easy fix, I can provide a PR for this. Will you accept an implementation that uses a 3rd party package for the recursive object merging or should this repo be without any production deps?

[robertrossmann]

I tried to implement configuration deep-merging just like Sails currently does, found an interesting problem.

In Trails, it seems to be encouraged to include some implementation details directly in the configuration, ie. here a logger instance is created directly in the configuration object. Doing a recursive merge of such objects seems wrong and could break stuff. Thus, I ask:

Is this a practice Trails is going to encourage? If so, how do we handle configuration data merging?

I see two ways forward:

1. Encourage only configuration data to be present in the config files
2. Do only shallow merge

Personally I prefer the first option since the latter would be quite prohibiting in terms of configuration cascading (ie. override a single value from a complex object for development environment).

[jaumard]

Maybe @tjwebb can give his thought here. Currently merge between config packs and default config is working and I think it merge everything. I think there only the logger who is create in the config file but in some case you can have require("toto") who can also be a complex object. I suppose in most case user will not override complex data under his environment config. He will do most of the job under the default config and only override few thing for env specific.

[jaumard]

@tjwebb I see you have put https://www.langa.io in production with Trails :D how did you fix this ? (if you did ^^)

[tjwebb]

I see you have put https://www.langa.io in production with Trails :D how did you fix this ? (if you did ^^)

No I did not resolve this issue yet. The config is very simple, and I use environment vars: https://github.com/langateam/api.langa.io/blob/master/config/email.js#L1-L5

re: https://github.com/trailsjs/trails/issues/102#issuecomment-195080167

This code will be completely removed in #145. Along with #121, I think this code is no longer relevant. (thanks @Alaneor)

Doing a recursive merge of such objects seems wrong and could break stuff.

Merging functions is like dividing by zero -- if we try to do it, very weird things will happen. In this sense, functions and all other non-object primitives (int, string, etc) are considered leaves in the tree, and they cannot be merged with other branches of the tree (object).

The behavior should be similar to Sails. I think there are some untested code areas, and some bugs have been able to sneak in. I will look at this tonight and try to fix and add tests.

[tjwebb]

The logic in the trailpack project is only merging the environment configs for the trailpacks, not the application :) So configs set in places that are not trailpack-specific (e.g., main.js) are not merged correctly.

Fix is in progress.