RFC: Make conventional directories for config and data the default

[wbinnssmith]

Following from #5336, this makes the typical directories for XDG and Windows conventions the default.

This RFC is really carried over from #5336, where we continued to use the existing locations inside ~/.config and punted on migrating users to these new locations.

A suggestion from the prior PR include moving the contents of the existing directories (should they exist), leaving a symlink in place.

I really like this approach, however there will be a cost to checking to see if these directories exist and leaving symlinks behind in a synchronous way to maintain compatibility with how we expose constants synchronously from a module.

Please chime in!

[wbinnssmith]

cc @bestander @kelseasy

[buildsize]

This change will decrease the build size from 10.45 MB to 10.45 MB, a decrease of 1.25 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	905.95 KB	905.78 KB	-167 bytes (0%)
yarn-[version].js	3.94 MB	3.94 MB	-413 bytes (0%)
yarn-legacy-[version].js	4.08 MB	4.08 MB	-405 bytes (0%)
yarn-v[version].tar.gz	911.31 KB	911.13 KB	-181 bytes (0%)
yarn_[version]all.deb	673.19 KB	673.08 KB	-112 bytes (0%)

[bestander]

@wbinnssmith, so in current implementation it does not handle migration?
I would not want to break people's global bins without a prompt.

[wbinnssmith]

yeah right now it doesn't. I just opened this up with the content of the initial version of the last PR so we can get a discussion going.

The simplest thing which would preserve the current behavior of having these values available at export time would be to synchronously stat the old locations and conditionally synchronously move it at startup time, then synchronously place a symlink to the new location.

There's likely a performance hit doing this on every invocation of yarn, but assuming the common case is a single stat, it may not be so bad.

How about I prototype that and report back with some benchmarks?

Note that after a number of releases we can remove the symlink, and then a number of releases after that we can remove the check all-together.

[bestander]

@yarnpkg/core, this looks like a PR for major version bump.
We don't have a feature that would migrate users of previous folders automatically and we don't want to leave orphaned folders if we introduce this change with a minor bump.
I like the change though, more conventional, less things to worry about down the path.

Opinions?

[kaylie-alexa]

I personally like the change, I think we should do a similar check like shouldWarnAboutConfig based on whether the directory is empty and give them guidance on what command to run instead of automatically moving or symlinking the files.

yarn/src/cli/index.js

Lines 251 to 253 in b2d3e1a

	if (warnAboutRunDashDash) {
	reporter.warn(reporter.lang('dashDashDeprecation'));
	}

I.e. with Yarn 2.0.0 onwards the directory will change to xyz, to avoid any conflicts in the future and remove this warning message you can type in `mv abc xyz` or re-add your global packages

[bestander]

I like the idea of soft warning.

[wbinnssmith]

I also think it's an elegant solution to warn the user, prompting them to run something manually rather than automatically moving things from underneath them.

Something like yarn migrate might be kind of neat rather than recommending the user run a specific move command, as we know exactly where things are and where they should be.

Unfortunately determining whether to display this warning will involve another fs stat (specifically a synchronous one if we want to remain compatible with the way constants work right now).

I'll push something up in the coming days that explores this. Looking forward to more thoughts!

[kaylie-alexa]

@wbinnssmith I think you're right that it'll no longer have to be a constant and have to be determined during runtime like we do for cache folder

yarn/src/config.js

Lines 298 to 316 in ce47045

	let preferredCacheFolders = constants.PREFERRED_MODULE_CACHE_DIRECTORIES;
	const preferredCacheFolder = opts.preferredCacheFolder || this.getOption('preferred-cache-folder', true);
	if (preferredCacheFolder) {
	preferredCacheFolders = [String(preferredCacheFolder)].concat(preferredCacheFolders);
	}
	const cacheFolderQuery = await fs.getFirstSuitableFolder(
	preferredCacheFolders,
	fs.constants.W_OK | fs.constants.X_OK | fs.constants.R_OK, // eslint-disable-line no-bitwise
	);
	for (const skippedEntry of cacheFolderQuery.skipped) {
	this.reporter.warn(this.reporter.lang('cacheFolderSkipped', skippedEntry.folder));
	}
	cacheRootFolder = cacheFolderQuery.folder;
	if (cacheRootFolder && cacheFolderQuery.skipped.length > 0) {
	this.reporter.warn(this.reporter.lang('cacheFolderSelected', cacheRootFolder));
	}

So I was thinking that if the previous directory does exist, we'd just fallback to existing directory and emit the warning message, then we actually replace the fallback and warning in 2.0. Wdyt?

[kaylie-alexa]

and, awesome job, thanks for the PR ❤️ 🎉

[Gudahtt]

@kaylie-alexa That approach sounds pretty reasonable to me.

@wbinnssmith are you still interested in working on this? Let us know if you'd like to hand this off to someone else.

[Siilwyn]

@Gudahtt I would propose to use env-paths and do something like the following:

const path = require('path');
const userHome = require('./user-home-dir').default;
const envPaths = require('env-paths');
const { existsSync } = require('fs');

const paths = envPaths('yarn', { suffix: '' });

export function getDataDir(): string {
  return maybePath(getOldPath('data')) || paths.data;
}

export function getCacheDir(): string {
  return maybePath(getOldPath('cache')) || paths.cache;
}

export function getConfigDir(): string {
  return maybePath(getOldPath('config')) || paths.config;
}

function maybePath(path) {
  if(existsSync(path)) {
    return path;
  } else {
    return false;
  }
}

Where getOldPath would return the old path like it is currently in src/util/user-dirs.js. If somebody is worried about performance could also memoize the calls, though I don't think it is needed here. Note that this would not be a breaking/major change since setups with old configs would still work fine.

[Siilwyn]

@Gudahtt @wbinnssmith thoughts?

[wbinnssmith]

@Siilwyn I've been thinking of taking this up again given Yarn v2 is approaching (or is at least planned). Happy to work together, or if you'd prefer to tackle this yourself that's fine too.

@Siilwyn I like that env-paths packages up this tricky logic in a reusable way, but it does default to the Mac-friendly paths which I don't know if we want -- it seems like XDG's .config and .local fit a CLI tool better. I'm also hesitant to change the paths yet again.

Given the typescript change and the upcoming plans for v2, is there anything we should keep in mind @kaylie-alexa and @arcanis?

Sorry for the delay.

[Siilwyn]

@wbinnssmith yeah would be happy to collab! I actually took a stab at this last week but got stuck at trying to write tests for both situations where: the old location should be used and when the new one should be used. I'll push the code, was looking into either dependency injection or using a Sinon sandbox.

Using env-paths is a huge win on all platforms in my book, following the OS standards should be default, same for CLI tools. I'm actually surprised env-paths methods are not in the Node.js standard library. That being said, I do not use MacOS so I don't have experience with the platform.

Would be good to know what is planned for version two, but remember this won't be breaking change if we fallback to the old location!

[Gudahtt]

Given the typescript change and the upcoming plans for v2, is there anything we should keep in mind @kaylie-alexa and @arcanis?

Not to speak for them, but I don't think the plans for v2 change anything about how we should proceed with this issue. This can be accomplished without a breaking change, and clearly is desired by a lot of people. It would be great to get this in v1. Yarn v1 will continue to be maintained for the foreseeable future.

I like that env-paths packages up this tricky logic in a reusable way, but it does default to the Mac-friendly paths which I don't know if we want -- it seems like XDG's .config and .local fit a CLI tool better

Interesting. This point was raised by @Siilwyn in the env-paths issue tracker, and the response was fairly convincing. Yet I do see a decent amount of people expressing a preference for using XDG on MacOS for CLI tools.

Using MacOS standards might be safer, as that's the option most likely to "just work". As alluded to by the env-paths maintainer, they've run into permissions problems in the past when using XDG on MacOS. Most people don't care where the config lives, as long as it works.

Though perhaps the permissions problems were overstated, or are no longer common? I don't have any recent experience with MacOS either, so I'd be interested to hear others perspectives on this.

Edit: Oh, I see what you mean now about not changing the path again. I had forgotten that Yarn already uses $HOME/.config on MacOS, and has since v1.51. That's a good point.

[Siilwyn]

Pushed my code so far at my fork on the env-paths branch, diff: master...Siilwyn:env-paths

The tests are mostly skipped, need to work on those and add tests to check if the fallback strategy works, I guess by mocking fs calls. Should I create a merge request for @wbinnssmith's branch? 🤔

(also I have no experience with flow so added any to get maybePath working) 🙈

[arcanis]

This can be accomplished without a breaking change, and clearly is desired by a lot of people. It would be great to get this in v1. Yarn v1 will continue to be maintained for the foreseeable future.

I have some concerns about putting this in the v1 - the problem I see is that if we change the directory paths, it will affect currently existing setups. For example Yarn will start looking for links in different directories than before, which could unexpectedly break installs. Same things for global installs, etc.

It's definitely something that we should do going forward, but I feel like the cons outweigh the cons to ship this without a major bump (especially when a major is so close). Opinions?

[Siilwyn]

@arcanis it won't affect existing setups since it will first use the 'old' paths. So only on new setups the change will have an effect.

[Siilwyn]

I have tested my branch by building the dist and switching to the binary in my day-to-day usage. One thing I noticed is that .yarnrc is not using the paths returned by user-dirs, it should be in the configuration directory but instead is written to the home directory. @wbinnssmith did you maybe already look into this?

[quintrino]

@Siilwyn Just checking how this feature is going, did you get to fully functional and it just needs to be merged?

[Siilwyn]

Hi @quintrino no I didn't, feel free to pick up this issue.

[soredake]

any progress?

[Siilwyn]

@soredake no, feel free to pick this up. I dropped it because I couldn't find the time. With Berry coming up / released I also have no idea how this will be done.

[jedahan]

I think this is supported now? 2d454b5