Skip to content
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.

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

feat: new switch v2 function #20

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
18 changes: 18 additions & 0 deletions config/env/v2-development.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* @fileOverview This is a variation of a SailsJS local.js file that uses env-lift to provide configuration overrides
* from environment variables.
*/
module.exports = require('../../index').switchV2(__filename, {
port: 1337,

environment: 'development',

models: {
connection: 'mySQL',
migrate: 'safe'
},

log: {
level: 'info'
}
});
17 changes: 17 additions & 0 deletions config/env/v2-internal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @fileOverview This is a variation of a SailsJS local.js file that uses env-lift to provide configuration overrides
* from environment variables.
*/
module.exports = require('../../index').switchV2(__filename, {
port: 6060,

environment: 'v2-internal',

log: {
level: 'silly'
},

foo: {
bar: 'v2-dummy'
}
});
43 changes: 41 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var path = require('path');
const path = require('path');

module.exports = {
/**
Expand Down Expand Up @@ -42,7 +42,7 @@ module.exports = {
}

// Get the file name from which config will be loaded.
var targetFile = require.resolve(path.join(process.cwd(), 'config', 'env', process.env.SAILS_ENV));
const targetFile = require.resolve(path.join(process.cwd(), 'config', 'env', process.env.SAILS_ENV));
Copy link

Choose a reason for hiding this comment

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

revert ? let's not make unnecessary changes


// if the parent file name from which this module is called is same as the targetFile name then no need
// to require the targetFile as we are already at the target File and we need that file's config.
Expand All @@ -57,6 +57,45 @@ module.exports = {
// called and that is dynamic, so we need to make sure we are requiring the correct parent file everytime.
delete require.cache[__filename];

// Finally return the target file's config.
return require(targetFile);
},
Comment on lines +61 to +63
Copy link

Choose a reason for hiding this comment

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

Wasn't this already present ? Why showing up as newly added 😕

Copy link
Author

Choose a reason for hiding this comment

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

GitHub diff is showing lines 92 and below as existing.


/**
* `delete require.cache()` might not work properly if there is any hook applied to require module, eg: require-in-the-middle
* switchV2() will expect parentFilename as an parameter and use that to compare with the targetFile.
*
* It will pick the env config based on the SAILS_ENV value. If a project has SAILS_ENV
* defined to some value then it expects that same file in the env/config file so that it
* will pick the config value from that file.
*
* @param env this is the current sails environment in which this module is used.
* @param parentFilename this is the filename of the module which is calling switchV2.
* @param defaultConfig this is the default config which is bases on NODE_ENV value
* @returns {config}
Copy link

Choose a reason for hiding this comment

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

For any other reviewers,
We're okay with code repetition because eventually, there's going to be one version only.

*
* @example
* // This is the env config file
* module.exports = require('sails-env-switch').switchV2(__filename, {
* port: 8080,
* name: 'Sample'
* });
*/
switchV2: function (parentFilename, defaultConfig) {
Copy link

Choose a reason for hiding this comment

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

Imo the stack approach is better, this we can keep the interface same.
Why didn't we go with that ?

If need be switch and switchV2 can be merged.
https://stackoverflow.com/a/63039252

cc: @kunagpal

// If SAILS_ENV is not defined, return the default config
if (!process.env.SAILS_ENV) {
return defaultConfig;
}

// Get the file name from which config will be loaded.
const targetFile = require.resolve(path.join(process.cwd(), 'config', 'env', process.env.SAILS_ENV));

// if the parent file name is same as the targetFile name then no need
// to require the targetFile as we are already at the target File and we need that file's config.
if (path.relative(targetFile, parentFilename) === '') {
return defaultConfig;
}

// Finally return the target file's config.
return require(targetFile);
}
Expand Down
Loading