Global Config at ${PREFIX}/etc/{nmp,yarn}rc #3958
Conversation
This searches for the global `rc` files at `${PREFIX}/etc/{npm,yarn}rc`, whereas previously it looked in `${PREFIX}/.{npm,yarn}rc`. By code inspection it looks like the NPM code does the same thing on windows and posix, but the documentation suggests it doesn't use the `/etc` path component. I haven't verified behaviour on windows.
src/registries/npm-registry.js
Outdated
| @@ -130,7 +130,7 @@ export default class NpmRegistry extends Registry { | |||
| const possibles = [ | |||
| [false, path.join(this.cwd, filename)], | |||
| [true, this.config.userconfig || path.join(userHome, filename)], | |||
| [false, path.join(getGlobalPrefix(), filename)], | |||
| [false, path.join(getGlobalPrefix(), 'etc', filename.slice(1))], | |||
There was a problem hiding this comment.
Please add a comment about the .slice(1) part above this line. Or even better, add example paths as comments:
./.npmrc
~/.npmrc
$PREFIX/etc/npmrc
There was a problem hiding this comment.
Actually to be properly correct it should probably take the RC name without the leading . and prefix it, rather than slicing it off. If we're going to do this properly I can try and take a look tomorrow at filling it out a bit.
I don't know anything about the Windows install of this or NPM though, so someone else will need to check that aspect of it.
There was a problem hiding this comment.
That sound great :) Your change seems okay on Windows too since it seems in-line with what NPM documents and it doesn't break AppVeyor builds.
|
Thanks a lot for the patch by the way! :) |
Moved the leading `.` from the config filename to be added by the location searcher. The alternative is to strip the leading `.` to create the global name, but that feels clumsy. This should also make it easier to make portable if the leading `.` is not used for hidden config files on other platforms. Added some comments explaining the path transformations.
Leading `.` is added now by `getPossibleConfigLocations` method.
|
I think this structure is cleaner, and I added some comments. |
|
I just noticed similar path searching behaviour in https://github.com/yarnpkg/yarn/blob/master/src/util/rc.js but I'm not sure where this code fits in. It does miss the |
Oh, that's quite concerning actually. Let me check that. |
|
@Parakleta I'll go ahead and merge your fix since it fixes an immediate problem. That said Would you be interested in taking a stab at it as a follow-up? PS: const home = isWin ? process.env.USERPROFILE : process.env.HOME;line should actually be const home = process.env.HOME || process.env.USERPROFILE;Sicne you can have a |
|
I think consolidating |
|
@Parakleta It's actually only used in two places so it shouldn't be too complicated. If you don't have time, that's okay but if you don't wanna try just because you think you'd get drowned we're here to help ;) |
**Summary**
This searches for the global `rc` files at `${PREFIX}/etc/{npm,yarn}rc`, whereas previously it looked in `${PREFIX}/.{npm,yarn}rc`. By code inspection it looks like the NPM code does the same thing on windows and posix, but the documentation suggests it doesn't use the `/etc` path component.
**Test plan**
N/A
This searches for the global RC files at
${PREFIX}/etc/{npm,yarn}rc, whereas previously it looked in${PREFIX}/.{npm,yarn}rc. By code inspection it looks like the NPM code does the same thing on both windows and posix, but the documentation for windows suggests it might not use the/etcpath component. I haven't verified behaviour on windows.Resolves #1931.