-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: develop
Are you sure you want to change the base?
Conversation
index.js
Outdated
* name: 'Sample' | ||
* }); | ||
*/ | ||
switchV2: function (parentFilename, defaultConfig) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Let's do all we can to avoid split functionality. As pointed out earlier, this module will be deprecated in a few months anyway.
- If API Observability has confirmed the above is an undigestable risk, let's provide a more descriptive name than
.switchV2
. No one adopting such a function will be aware of the original thread that sparked this change, nor will they have the time to sift through the code to try and comprehend howV2
differs from the original.
I have no other review comments, merge and release as you see fit 🙂
index.js
Outdated
@@ -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)); |
There was a problem hiding this comment.
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
// Finally return the target file's config. | ||
return require(targetFile); | ||
}, |
There was a problem hiding this comment.
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 😕
There was a problem hiding this comment.
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.
* will pick the config value from that file. | ||
* | ||
* @param defaultConfig this is the default config which is bases on NODE_ENV value | ||
* @returns {config} |
There was a problem hiding this comment.
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.
No description provided.