feat(config): use XDG config specification #3674
Conversation
|
The only concern is the extra clutter, can we have it in one place? |
|
Do you mean between the two registry files? I tried to put everything in |
src/util/rc.js
Outdated
| @@ -49,6 +49,7 @@ export function findRc(name: string, parser: Function): Object { | |||
| } | |||
|
|
|||
| if (home) { | |||
| addConfigPath(process.env.XDG_CONFIG_HOME || join(home, '.config', name, `${name}rc`)); | |||
There was a problem hiding this comment.
not clear why process.env.XDG_CONFIG_HOME OR home/.config is used now
src/registries/yarn-registry.js
Outdated
| @@ -41,7 +41,7 @@ export default class YarnRegistry extends NpmRegistry { | |||
| constructor(cwd: string, registries: ConfigRegistries, requestManager: RequestManager, reporter: Reporter) { | |||
| super(cwd, registries, requestManager, reporter); | |||
|
|
|||
| this.homeConfigLoc = path.join(userHome, '.yarnrc'); | |||
| this.homeConfigLoc = process.env.XDG_CONFIG_HOME || path.join(userHome, '.config', 'yarn', 'yarnrc'); | |||
There was a problem hiding this comment.
Why process.env.XDG_CONFIG_HOME can't define userHome or config.userConfig
There was a problem hiding this comment.
The problems are:
- we have multiple places that define from which folder the config will be read and reading it from process.env.XDG_CONFIG_HOME in a few places makes it even harder to reason
- I think we need a basic e2e test for this feature to make sure this won't regress
Only add paths where needed, keeping all possibilities that were offered before
|
The change should be far less intrusive now. I'll look at how e2e tests are done when I have some time to dedicate to this. |
|
Yeah, that is way better, thanks @Michael-Zapata. |
With `~/.yarnrc` being the historic location, breakage should not occur for current users
|
Thanks, @Michael-Zapata. Can you express the test similar to this d709c2f? |
|
I pushed to see if results were different in the CI, but that test is failing in local, as in both checks. I can get behind one of them failing, but the two of them seems rather strange. I should dig further before doing tests as this is obviously not working. :) |
|
@Michael-Zapata the CI seems fine. Do you need to work on this still or can we merge? @bestander any more concerns about this PR or do you think it is ready? |
|
Hey, sorry for not getting back on that. The test that was added isn't running in the CI from what I can see, as it fails completely locally. I didn't have the time to take a look at why it's failing and that should be fixed before merging as this is not functional at the moment. |
|
@Michael-Zapata alright, how can I assist you with that? :) |
|
I just need to investigate to understand why this isn't working, I am not really familiar with the codebase and I won't have the time to do that before this week-end. :/ Some pointers on where this might fizzle would be very much welcome though! |
I don't know that end-to-end script either so can't really help quickly :( That said once I have time to get back to this, I can help if you don't beat me to it :) |
|
Any chance of moving the |
|
Some time has passed, let's close the PR as there are too many conflicts. |
As per XDG Base Directory Specification, prioritise the environment variable
$XDG_CONFIG_PATHwhen possible, and use$HOME/.config/as a fallback.Backward-compatibility is handled to avoid unnecessary breakage, this is a first step towards full compliance with the spec.
Related to #2334 (But only solves part of it)