Skip to content

Stop caching config files #194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bajtos merged 1 commit into from
Jul 13, 2016
Merged

Stop caching config files #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/config-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

var cloneDeep = require('lodash').cloneDeep;
Copy link
Copy Markdown

@richardpringle richardpringle Jul 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash has a lot of useful utility functions, might make more sense to require it on its own so that someone can use other lodash functions without changing this line.

Copy link
Copy Markdown
Contributor

@davidcheung davidcheung Jul 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another note on lodash, we had previous goals to shrink bundle size, would this be going against that,
but CI being more critical, another option would be: we are using traverse.clone() in datasource-juggler

Copy link
Copy Markdown
Member Author

@bajtos bajtos Jul 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash has a lot of useful utility functions, might make more sense to require it on its own so that someone can use other lodash functions without changing this line.

I am just following the style seen in existing code, see e.g.

loopback-boot/lib/compiler.js

Line 7 in 296c385

var cloneDeep = require('lodash').cloneDeep;

another note on lodash, we had previous goals to shrink bundle size, would this be going against that,

Bundle size is important, thank you for raising this concern. The browser bundle contains only the source code from lib/executor.js, config loader is executed during compilation, therefore it's ok to use lodash here.

Copy link
Copy Markdown
Member Author

@bajtos bajtos Jul 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidcheung @richardpringle PTAL ☝️

Copy link
Copy Markdown

@richardpringle richardpringle Jul 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand by what I said but maybe that's just my preference.

It shouldn't stop this PR from being landed though.

var fs = require('fs');
var path = require('path');
var debug = require('debug')('loopback:boot:config-loader');
Expand Down Expand Up @@ -125,11 +126,12 @@ function findConfigFiles(appRootDir, env, name) {
*/
function loadConfigFiles(files) {
return files.map(function(f) {
var config = require(f);
var config = cloneDeep(require(f));
Object.defineProperty(config, '_filename', {
enumerable: false,
value: f,
});
debug('loaded config file %s: %j', f, config);
return config;
});
}
Expand Down
37 changes: 37 additions & 0 deletions test/config-loader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright IBM Corp. 2014,2016. All Rights Reserved.
// Node module: loopback-boot
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

var configLoader = require('../lib/config-loader');
var fs = require('fs-extra');
var path = require('path');
var expect = require('chai').expect;
var sandbox = require('./helpers/sandbox');
var appdir = require('./helpers/appdir');

describe('config-loader', function() {
beforeEach(sandbox.reset);
beforeEach(appdir.init);

it('does not cache loaded values', function() {
appdir.createConfigFilesSync();
appdir.writeConfigFileSync('middleware.json', {
'strong-error-handler': { params: { debug: false }},
});
appdir.writeConfigFileSync('middleware.development.json', {
'strong-error-handler': { params: { debug: true }},
});

// Here we load main config and merge it with DEV overrides
var config = configLoader.loadMiddleware(appdir.PATH, 'development');
expect(config['strong-error-handler'].params.debug, 'debug in development')
.to.equal(true);

// When we load the config file again in different environment,
// only the main file is loaded and no overrides are applied.
config = configLoader.loadMiddleware(appdir.PATH, 'production');
expect(config['strong-error-handler'].params.debug, 'debug in production')
.to.equal(false);
});
});