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

follow links #869

Closed
wants to merge 1 commit into from
Closed

follow links #869

wants to merge 1 commit into from

Conversation

KillWolfVlad
Copy link
Contributor

@KillWolfVlad KillWolfVlad commented Sep 28, 2018

yarn ~v1.10.x changed bin linking for global packages (I can't find commit with that changes).

How yarn link bin after $ yarn global add package?

$ file ~/.yarn/bin/package
~/.yarn/bin/package: symbolic link to ../../.config/yarn/global/node_modules/.bin/package
$ file ~/.config/yarn/global/node_modules/.bin/package
~/.config/yarn/global/node_modules/.bin/package: symbolic link to ../package/bin/package.js

But commander.js read link only once time. And sub-commands not work, because in directory ~/.config/yarn/global/node_modules/.bin other files doesn't exists.

My solution: follow links and get real file name.

In GNU/Linux shell command readlink -f implements it:

$ readlink -f ~/.yarn/bin/package
~/.config/yarn/global/node_modules/package/bin/package.js

For Node.js I found package readlink with almost similar behavior (from description).

Close #866

@abetomo abetomo requested review from abetomo and roman-vanesyan and removed request for abetomo September 28, 2018 08:02
@KillWolfVlad
Copy link
Contributor Author

KillWolfVlad commented Oct 5, 2018

Rebased with master (previous commit here)

Rebased with master (previous commit here)

Rebased with master (previous commit here)

Rebased with master (previous commit here)

index.js Outdated
@@ -8,6 +8,7 @@ var path = require('path');
var dirname = path.dirname;
var basename = path.basename;
var fs = require('fs');
var readlink = require('readlink');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please inline the synchronous part of the readlink module; as module itself introduces an async module which is unnecessary for the commander and only introduce a ton of unused kb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that CTRL+C/CTRL+V code from readlink is good idea, because we must also provide tests and support this legacy code. If you don't like async module maybe better send PR to readlink repo to remove it from dependencies.

Copy link
Collaborator

@roman-vanesyan roman-vanesyan left a comment

Choose a reason for hiding this comment

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

Hello there! Sorry me make you waiting for so long. Can you, please, fix outlined part in the review comments.

@shadowspawn
Copy link
Collaborator

Whether to introduce dependencies is a tricky question. I'm still thinking about the tradeoffs so won't offer an opinion at this time, just add some size info.

$ npm init -y
$ npm install commander
$ du -d 1 -h node_modules/
 76K	node_modules//commander
 76K	node_modules/
$ npm install readlink
$ du -d 1 -h node_modules/
4.8M	node_modules//lodash
 76K	node_modules//commander
844K	node_modules//async
 24K	node_modules//readlink
5.8M	node_modules/

@KillWolfVlad
Copy link
Contributor Author

fixed in #935

@shadowspawn
Copy link
Collaborator

Thanks @KillWolfVlad

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

4 participants