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

Fix global add linking issue #1889

Merged
merged 1 commit into from
Nov 16, 2016
Merged

Conversation

torifat
Copy link
Member

@torifat torifat commented Nov 16, 2016

Summary

~/.config/yarn/global/node_modules/.bin was not creating.

Fixes #1886

@bestander bestander merged commit b76ac93 into yarnpkg:master Nov 16, 2016
@bestander
Copy link
Member

I'll do a 0.17.3 today

bestander pushed a commit that referenced this pull request Nov 16, 2016
Conflicts:
	__tests__/commands/global.js
@torifat torifat deleted the fix/global-bin branch November 17, 2016 02:49
await config.init({cwd: config.globalFolder});
await config.init({
cwd: config.globalFolder,
binLinks: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we take the value of config.binLinks instead of hard coding the value of binLinks to true.
What about all other option, config.init will set all other Boolean options to false, or to the default value from constants.

diff --git a/src/cli/commands/global.js b/src/cli/commands/global.js
--- a/src/cli/commands/global.js
+++ b/src/cli/commands/global.js
@@ -32,10 +32,15 @@ class GlobalAdd extends Add {
 const path = require('path');

 async function updateCwd(config: Config): Promise<void> {
-  await config.init({
-    cwd: config.globalFolder,
-    binLinks: true,
-  });
+  const options = {};
+  for (const key of Object.keys(config)) {
+    const val = config[key];
+    if (typeof val == 'string' || typeof val == 'boolean') {
+      options[key] = val;
+    }
+  }
+  options.cwd = config.globalFolder;
+  await config.init(options);
 }

 async function getBins(config: Config): Promise<Set<string>> {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we take the value of config.binLinks instead of hard coding the value of binLinks to true.

That would be awesome but for some reason config.binLinks is false here. Since this is a major blocker for lots of people so I decided to fix it with true for the time being.

What about all other option, config.init will set all other Boolean options to false, or to the default value from constants.

I tried what you are trying to do but it will break some other things. For example prefix. I have already asked @kittens about it. We might need some refactoring here.

@onemen
Copy link

onemen commented Nov 17, 2016

This is odd, for me config.binLinks is always true, unless i use --no-bin-links from the command line
I did not find the place in the code to use binLinks from the config file.

The configuration is confusing, it should take the configuration in this order

  • command line
  • config file
  • default

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.

3 participants