Skip to content
This repository has been archived by the owner on Feb 15, 2021. It is now read-only.

find: Start from process.cwd() instead of __dirname. #12

Closed
wants to merge 1 commit into from

Conversation

evocateur
Copy link

This supports a globally-installed dependency on config-chain being able to traverse a more sensible directory tree, instead of /usr/local/*. This makes the cc.find(".myconfigrc") result more consistent with expectations when run by a global utility.

beautifier/js-beautify#228 demonstrates this problem.

This supports a globally-installed dependency on config-chain being able to traverse a more sensible directory tree, instead of /usr/local/*.
@dominictarr
Copy link
Collaborator

ah, that does make more sense, but you know what? config-chain is getting messy, what you should do, is just publish this as a separate module. really, the start directory should be a parameter, anyway.

Also, are you using other config-chain features?
Can I also suggest using rc it's better than config-chain because it
is completely hardwired, so everyone who uses it ends up with the same configuration patterns!

Instantly, they are freed from having to document precisely how their configuration works.
(but it has the flexibility to set configuration by multiple files (in the usual locations), command line arguments, or enviroment variables!

@bitwiseman
Copy link

Any reason why rc is a separate module? It is just a thin wrapper around config-chain (and directly depends on it). Why not just fold that functionality into config-chain?

@evocateur
Copy link
Author

@dominictarr I think the only reason I decided not to use rc was its conflation of optimist (we use nopt). It would be nice if it wasn't so tightly coupled, but I recognize it is opinionated code.

(I went down the path of making the starting directory a parameter, but went for the simplest change instead)

@dominictarr
Copy link
Collaborator

Because the point of rc is that it is hardwired. It's completely unconfigurable, except for an appname and defaults.
I just wanted to package the most sensible config-chain settings, and keep them separate.

Having that as part of config-chain wouldn't be the same.

Also, now rc uses deep-extend to merge configuration sources, so, while it still uses config-chain, it only uses one small utility from it.

@dominictarr
Copy link
Collaborator

Configuration is the sort of thing that strongly benefits from opinions, and picking just one way to do it.
Don't get me wrong -- I wear odd socks, and funny clothes, and do many things just for the sake of doing it differently.
But I don't want to ever have to think about how configuration is loaded,
and worse: I don't want to ever have to read custom configuration loading code.

Even @isaacs has stated, that if he was to start npm over, he'd just use rc.

Anyway, this is technically a breaking change, so I'd want to hear from @isaacs about this change...

(by the way, I've just updated rc to remove it's config-chain dep)

what does nopt do that optimist doesn't do?

@isaacs
Copy link
Collaborator

isaacs commented Apr 8, 2013

nopt does extensible param validity checking.

IMO, config-chain should not ever do anything with files. It should strictly be a thing to chain together config objects, and limit itself to that. npmconf only uses the ConfigChain class as a slightly more customized version of ProtoList. The file-loading opinions belong in rc (the "config file loader" module) rather than in config-chain (the "config object resolver" module).

If you make breaking changes, just bump the major version. npmconf is using ~1.1.1, and probably won't ever have to upgrade, so go nuts :)

@dominictarr
Copy link
Collaborator

so, I no longer have anything that depends on config chain, so we can remove the file stuff,
and bump the major. that would obliterate the place this patch was gonna go, though...

but, this pull request can be published as a new module.

@evocateur
Copy link
Author

I totally respect opinionated software, I just don't like having two libraries trying to do the same thing in my module dependencies. I appreciate the changes made to rc today, and I'll explore publishing another module to incorporate the functionality we need.

Thanks for all the input!

@evocateur evocateur closed this Apr 8, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants