Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prefer local config to home and home to global #951

Merged
merged 2 commits into from
Oct 15, 2016

Conversation

FLGMwt
Copy link
Contributor

@FLGMwt FLGMwt commented Oct 13, 2016

Summary
#949

Right now, config resolution favors global, and only uses more local configs if those contain keys undefined in more global configs.

For example,

If i have

#~/.yarnrc
turtles up

and

#./.yarnrc
turtles down

then yarn config list outputs the following:

> yarn config list
yarn config v0.15.1
info yarn config
{ turtles: 'up',
  'version-tag-prefix': 'v',
  'version-git-tag': true,
  'version-git-sign': false,
  'version-git-message': 'v%s',
  'init-version': '1.0.0',
  'init-license': 'MIT',
  'save-prefix': '^',
  'ignore-scripts': false,
  'ignore-optional': false,
  registry: 'https://registry.yarnpkg.com',
  'user-agent': 'yarn/0.15.1 npm/? node/v6.2.1 win32 x64' }
info npm config
{ progress: false }
Done in 0.11s.

Test plan
With changes reordering the config resolution, yarn config list outputs the following:

> yarn config list
yarn config v0.15.1
info yarn config
{ turtles: 'down',
  'version-tag-prefix': 'v',
  'version-git-tag': true,
  'version-git-sign': false,
  'version-git-message': 'v%s',
  'init-version': '1.0.0',
  'init-license': 'MIT',
  'save-prefix': '^',
  'ignore-scripts': false,
  'ignore-optional': false,
  registry: 'https://registry.yarnpkg.com',
  'user-agent': 'yarn/0.15.1 npm/? node/v6.2.1 win32 x64' }
info npm config
{ progress: false }
Done in 0.11s.

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 13, 2016

Fixes #949

@jgoz
Copy link
Contributor

jgoz commented Oct 13, 2016

@FLGMwt I think your example is accidentally confusing - one of those config files should be turtles down.

@jgoz
Copy link
Contributor

jgoz commented Oct 13, 2016

This will fix the behavior for setting config values, but it won't prevent reading/parsing the same file multiple times (potentially). Heck, even starting at {cwd}/.. instead of {cwd} when adding to the possibles list would prevent {cwd}/.*rc from being read twice each time.

const foldersFromRootToCwd = this.cwd.split(path.sep).slice(0, -1); // start at {cwd}/..
while (foldersFromRootToCwd.length > 1) {
  possibles.push([false, path.join(foldersFromRootToCwd.join(path.sep), filename)]);
  foldersFromRootToCwd.pop();
}

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 13, 2016

good catch on the example @jgoz. Fixed

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 13, 2016

And wrt foldersFromRootToCwd, good point. Thought I'm not sure why it's there. npm config doesn't search to root.

I just tried nesting a project with a .npmrc inside of a project with an .npmrc and when I removed the inner .npmrc, the one in the parent directory wasn't respected. (with npm config list, I mean)

@jgoz
Copy link
Contributor

jgoz commented Oct 13, 2016

I just tried nesting a project with a .npmrc inside of a project with an .npmrc and when I removed the inner .npmrc, the one in the parent directory wasn't respected. (with npm config list, I mean)

Yeah, that's because the home and global configs would take precedence (since it's using defaults and not Object.assign).

Thought I'm not sure why it's there. npm config doesn't search to root.

Right, I looked that up too and npm uses the project config, home config, global config, and built-in config (in that order). Not that yarn has to behave the same as npm here — resolving config files using the module resolution search path has a nice symmetry to it, but values are currently being selected in an unintuitive way, IMO.

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 13, 2016

Sorry for not being clear: my last comment was me checking the behavior of npm and not yarn. I was making sure that npm didn't actually look up from . to root.

Before I fix foldersFromRootToCwd to behave as it should (even though it doesn't reflect npm behavior), I'd like to get input on whether we want to keep foldersFromRootToCwd at all.

@jgoz
Copy link
Contributor

jgoz commented Oct 13, 2016

Sorry for not being clear: my last comment was me checking the behavior of npm and not yarn. I was making sure that npm didn't actually look up from . to root.

My fault, I'm so eager to drop npm that my eyes are doing s/npm/yarn/g everywhere 😁

Before I fix foldersFromRootToCwd to behave as it should (even though it doesn't reflect npm behavior), I'd like to get input on whether we want to keep foldersFromRootToCwd at all.

I would vote to remove it to keep things simple, but it could be useful/used in multi-repos (like babel, react) to have a single config for multiple sub projects.

@whitelynx
Copy link

For monorepos/multi-repos, it might be good to just have a per-file option like inherit: true that signals that we should look in parent directories.

On the other hand, if the files we're loading are just .npmrc files (and we don't have a yarn-specific config file), I think we should handle them the same way npm does, to reduce confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants