-
Notifications
You must be signed in to change notification settings - Fork 32
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: Create types and js for nested configs #36
feat: Create types and js for nested configs #36
Conversation
5903a5f
to
39821eb
Compare
src/createMergedConfig.ts
Outdated
export const getAllConfigPath: <T extends NonConfigEnv>( | ||
process: T | ||
) => Array<string> = process => | ||
glob.sync(`**/${baseConfigPathName}`, { |
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.
take the glob param as a parameter to the cli.
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.
move this function to its own file.
src/createMergedConfig.ts
Outdated
process: T | ||
) => Array<string> = process => | ||
glob.sync(`**/${baseConfigPathName}`, { | ||
ignore: ['node_modules/**'], |
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.
ignored
paths should be configurable.
webpack.ts
Outdated
@@ -20,4 +20,4 @@ export const NodeConfigTSPlugin: { | |||
} = R.compose( | |||
setConfigResolver, | |||
setGlobalConfigPlugin | |||
) | |||
) as {(config: Configuration): Configuration} |
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.
why do we need type-casting here?
@@ -25,24 +25,30 @@ export type NonConfigEnv = { | |||
} | |||
|
|||
export const configPaths = <T extends NonConfigEnv>( |
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.
Add a JSDoc explaining what this function is doing.
Add information about each argument that is being passed.
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.
/**
* Returns the paths for all the config files — {default, env, deployment, user} etc.
* @param process - Used to know which file to load. For eg. if env=production, the env config will be env/production.json.
* @param root - // TODO
*/
src/configPaths.ts
Outdated
@@ -25,24 +25,30 @@ export type NonConfigEnv = { | |||
} | |||
|
|||
export const configPaths = <T extends NonConfigEnv>( | |||
process: T | |||
process: T, | |||
subPath?: string | |||
): ConfigTypes => { | |||
const baseDIR = baseConfigPath(process) |
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.
baseConfigPath
should not be dynamic any more because its being passed as an argument to this function.
src/createTypeAndJS.ts
Outdated
/** | ||
* Write typedef and js for each path in nested config | ||
*/ | ||
export const writeConfigFilesToSystem = () => { |
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.
split this into two separate functions/files.
src/createTypeAndJS.ts
Outdated
const getJsFileBuffer = (p: string) => | ||
defaultJsFileContent.concat(` mainConfig['${p}']`) | ||
|
||
const getTsFileBuffer = (config: 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.
This seems like a pure function. We can write unit tests easily for it.
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.
config
should be object
src/loadFileConfigs.ts
Outdated
) | ||
return itar(configPaths(process)) | ||
} | ||
): Configurations<ConfigTypes> => readConfigFiles(configPaths(process)) |
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.
this change seems unnecessary. Can you confirm?
If not needed can you revert this file.
index.ts
Outdated
|
||
import {mergeAllConfigs} from './src/mergeAllConfigs' | ||
export const config: Config = mergeAllConfigs(process) | ||
export const config: Config = createMergedConfig(process) |
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.
This rename is not needed.
68cdfee
to
b8a61ea
Compare
import {generateJsFiles} from './generateJsFiles' | ||
import {generateTypeDefFiles} from './generateTypeDefFiles' | ||
|
||
process.env['BOOTSTRAP'] = 'true' |
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.
Never change env variables.
process.env['BOOTSTRAP'] = 'true' | ||
|
||
generateTypeDefFiles() | ||
generateJsFiles() |
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.
generateTypeDefFiles
& generateTypeDefFiles
should only use the default.json
to create files.
|
||
export const checkIfBootstrap: <T extends NonConfigEnv>( | ||
process: T | ||
) => boolean = process => process.env['BOOTSTRAP'] === 'true' |
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.
this file can be removed.
return glob.sync(`**/${includePattern}`, { | ||
ignore: getIgnorePatterns(process).map((x: string) => x + '/**'), | ||
cwd: process.cwd() | ||
}) |
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.
filter out file paths that don't have default.json
in them.
process: T | ||
) => Array<string> = process => { | ||
const includePattern = getIncludePattern(process) | ||
return glob.sync(`**/${includePattern}`, { |
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.
don't change the passed user pattern.
|
||
export const getIncludePattern: <T extends ProcessArgv>( | ||
process: T | ||
) => string = process => minimist(process.argv)['pattern'] || DEFAULT_BASE_DIR |
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.
all exported functions should be in their files.
export const checkIfDefaultJson = <T extends NonConfigEnv>( | ||
process: T, | ||
p: string | ||
) => fs.existsSync(path.resolve(process.cwd(), `${p}/default.json`)) |
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.
you might not need this function any more since the paths will only contain config/default.json
"import {config as mainConfig} from 'node-config-ts' \n export const config =" | ||
|
||
export const getJsFileBuffer = (p: string) => | ||
defaultJsFileContent.concat(` mainConfig['${p}']`) |
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.
this will become a problem when u change the config directory's path.
import {loadCLIConfigs} from './loadCliConfigs' | ||
import {checkIfBootstrap} from './checkIfBootstrap' | ||
|
||
export const mergeFileConfigsForPath: <T extends NonConfigEnv>( |
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.
add JSDoc
357d87a
to
0dbe671
Compare
Outdated. Closing for now. |
Solves #37