Local config #1265
Local config #1265
Conversation
the with this the user can invoke any config command and choose to read/write to the local config object instead of the global
local configuration can exist in either /.jspmrc or /.config/.jspmrc
|
|
||
| // If the user wants to save options to a local configuration file that is not | ||
| // created, we need to create it and extend it with the default config. | ||
| if ( saveLocal && !config.local.path ) { |
There was a problem hiding this comment.
I'm just wondering about this logic, which will cause private data in the global config file to persist into the local project, potentially exposing passwords to version control by mistake.
There was a problem hiding this comment.
Sorry, I see this is just the default config being surfaced. I'm still not sure I understand why this is necessary though, if config is considered as the superposition of global and local configuration?
There was a problem hiding this comment.
Looking at it now, a month later, I share your confusion. Should work fine without it...
I'll make sure to test and confirm, and will update the PR if everything checks out!
There was a problem hiding this comment.
Ok, found the reason why I added it... Without that if clause, if I try to save local config in a dir where no local config file exists, L199 will throw because config.local.path is undefined.
The reason is that config.local.path, set on L54 via getLocalConfigPath(), will be not be set if no local config file exists. So in the if, I getDefaultLocalConfigPath() to know where to save the generated config file.
That config.local.path is undefined is used to know which config to load (L87), that's why I don't set it to a default value in getLocalConfigPath.
Does that make sense to you @guybedford?
There was a problem hiding this comment.
Surely the default localConfigPath can be implied as the path.resolve(config.pjsonPath, '.jspmrc')?
|
Thanks so much, will be great to get this in. |
|
@guybedford I'm the one who should be thankful! We use this every day at work and it is really working great for us! |
|
Glad to hear that. I hope you don't mind, but because every line of code added is more future maintenance it will need to be very carefully reviewed to ensure code complexity is kept to a minimum. |
|
Don't mind at all, I'd be worried if you didn't scrutinize it :) I guess what you're getting at is that I'm not sure that I follow you on the On a side note, I did find that I used some pretty stupid variable names in registry.js. Will fix and push the changes. |
|
Thought of another thing... If I create / config a registry and have a local config file, should the config be saved locally by default, or should it require the If locally by default, should there also be a I'm leaning towards the latter, it would make sense for local to be default if I have a default config available. But it should also possible to save globally from a locally configured project. |
| @@ -543,7 +543,7 @@ process.on('uncaughtException', function(err) { | |||
| return ui.log('warn', 'You must provide an registry name to create.'); | |||
| if (!args[3]) | |||
| return ui.log('warn', 'You must provide the registry module name to generate from.'); | |||
| return Promise.resolve(registry.create(args[2], args[3])) | |||
| return Promise.resolve(registry.create(args[2], args[3], undefined, options.local)) | |||
There was a problem hiding this comment.
Let's alter the signature rather to return Promise.resolve(registry.create(args[2], args[3], options.local))
|
I'm thinking it might be nicer to use a getter / setter global API along the lines of: // automatically gives the config value from local or global as appropriate
globalConfig.get('some.property.value');
// local flag is optional
globalConfig.set('some.property.value', value, local);We then ensure that all config operations from |
|
I think the default should always be to save to global config, unless |
|
This would be great! |
|
The changes in global-config.js made for a problematic merge. So instead of trying to figure out how to merge properly, in a codebase that I'm not 100% comfortable in, I'll close this PR and take another stab at it with the latest updates as base. |
|
Did functionality to accomplish this ever get merged? I can't figure out how to build on our build server as we only allow outbound github downloads via our cache. |
|
Local config would be very handy for me in my CI environment. |
This is a proposed fix for issue #190.
It creates a
--localflag forjspm configandjspm registry. If the--localflag is provided the configuration will be saved in./.jspmrcor./.config/.jspmrc, in that order.I couldn't really wrap my head around if and how to add tests for the changes, so no tests are added. I'd be happy to fix this if there is a standard to follow.