Nested executables fix

[jkimbo]

Summary

In npm 3, when installing a package where it's dependencies defined some bin scripts, those scripts would be symlinked to the top level node_modules/.bin/ directory so that the application can use them. Currently yarn doesn't do that so this PR fixes that issue.

(First attempt at a PR so please let me know if I've missed anything or if anything is wrong with the test!)

Related issue: #760 and credit to @DanielZlotin for pointing me to the right place

Test plan

$ mkdir tmp
$ cd tmp/
$ yarn add standard
$ ls node_modules/.bin/
  acorn  eslint  esparse  esvalidate  js-yaml  mkdirp  rimraf  shjs  standard  strip-json-comments

[sebmck]

This behaviour seems undesired. I'd rather we kept the current behaviour than follow npms current semantics.

[corysimmons]

Dependencies are dependencies. If a CLI is part of a dependency, then we're dependent on that CLI being available and shouldn't have to do more work to ensure all those dependent pieces are available.

What is Yarn's approach to this dilemma?

If there's not one, there seems to be a lot of people interested in this functionality: #760

[DanielZlotin]

@kittens well then you are causing migration problems for people that want to move from npm to yarn...
In fact, now dependencies can't run their postInstall scripts from the root dir as they expect this contract to hold.

[bitfrost]

@kittens Can you explain why this behavior is un-desired? #760 is the one issue keeping me from moving my org to yarn with out having to make a ton of changes. I think you will find a lot of projects in the community expect "npm run X" to work where is X is a bin script exported by a transitive dependency.

[djMax]

I see the argument on it being undesirable, because it means you have a direct dependency that you're not expressing that way. But I think the problems that will be caused by this major difference in behavior from npm are probably larger than the pedantic gains...

[rafayepes]

What's the problem with adding the dependency explicitly?

[djMax]

It's not a problem with that as much as a problem moving from npm to yarn.

[bitfrost]

One of the things I have been doing for internal projects at my company is curating a shared set of dependencies and build scripts via a single package "A" (think something like create-react-app) Internal teams depend on package A with a semantic version and they get majority of their dependencies via A and leverage the tools (executables) A depends on. In order to maintain this centralized sharing I will have do some work (add a post install processes) or change this sharing scheme. All doable with some effort, but to echo @djMax " But I think the problems that will be caused by this major difference in behavior from npm are probably larger than the pedantic gains..."

[SimonDegraeve]

@bitfrost we have the same process at my company. As a workaround I use require.resolve() to find the correct executable path.
Example: require.resolve('.bin/eslint')

You can have a look at https://github.com/SimonDegraeve/kode

[corysimmons]

In cases like these... why not just default to whatever you want and offer a flag to override that decision?

yarn add HasALotOfDependenciesWithCLIs --DontMakeMeHaveToAddAllOfThemExplicitly

[palmerj3]

I think adding tighter controls around ./node_modules/.bin is desireable. Right now it's a miracle that .bin conflicts are not all that common with NPM.

[ericclemmons]

I wanted to explain my use-case, as this PR may solve it.

@terse/webpack is largely an API wrapper around Webpack that the webpack.config.js files use (with optional presets to do complex things like long-term caching, etc.), but the package.json scripts such as npm start still runs webpack, since the project's goals aren't to replace webpack, but simplify the API to configure it. (It doesn't touch the binary)

What's important is that @terse/webpack has a direct dependency on webpack@version that's installed on the user's behalf to ensure that subtle differences between Webpack versions don't break functionality & presets.

As webpack ships new versions, @terse/webpack verifies that presets & features continue working, & then ships the updated webpack version with a patch release.

This was a legitimate solution after, webpack@beta.22 where the module.loaders API changed.

I agree that installing binaries for sub-dependencies risks collision, but I'm not sure how libs like @terse/webpack can operate with yarn unless they explicitly re-list their dependencies' binaries for inclusion in ./node_modules/.bin.

[corysimmons]

Unsubscribing and going to continue using npm. I'd really appreciate it if someone would ping me if this PR gets through or someone provides some docs for the official approach to this use case.

Not trying to be sassy/snarky/demanding. Thanks for your hard work.

[frederickfogerty]

Without trying to weigh in on a side of this debate, I would like to ask if there was a recommended way to re-list the dependencies in a package.json when using approaches such as as @ericclemmons's and @bitfrost's.

Right now the workaround I'm using is:

// @company/typescript/package.json

...
"bin": {
  "tsc": "../../typescript/bin/tsc", // reference consumer module's root node_modules
  ...
}

Is there a recommended way to do this? There is a problem in this scenario if the dependency is installed in the nested node_modules folder, rather than as a sibling, for example if there were conflicting versions.

If this PR is rejected (and the current behaviour kept, as @kittens preferred), I would appreciate it if yarn would document/support a way to do this.

[dexteryy]

For migrating from npm to yarn, I have to modify all npm scripts in package.json, use more explicit code instead of more abstract one. As a result, my npm scripts now are depend on the internal implementation of other npm packages.

For example:

dexteryy/webcube-example@4cba98d

Before using yarn:

"lint": "gulp --gulpfile node_modules/webcube/configs/gulpfile.babel.js check:all",
"dev:dashboard": "webpack-dashboard -- node node_modules/webcube/configs/devServer.js",
"new": "plop --plopfile=node_modules/webcube/configs/plopfile.babel.js",

After using yarn:

"lint": "./node_modules/gulp/bin/gulp.js --gulpfile node_modules/webcube/configs/gulpfile.babel.js check:all",
"dev:dashboard": "./node_modules/webpack-dashboard/bin/webpack-dashboard.js -- node node_modules/webcube/configs/devServer.js",
"new": "./node_modules/plop/plop.js --plopfile=node_modules/webcube/configs/plopfile.babel.js",

plop/plop.js and webpack-dashboard/bin/webpack-dashboard.js are internal implementation. They are likely be different and be changed.

[djMax]

There is theoretically another way to solve this, which is to have yarn run be aware of the unhoisted binaries, maybe via yarn.lock?

[djMax]

Given the size of this PR, there is of course another option... If anybody else is up for a fork, I am.

[ericclemmons]

It sounds like there are others that rely on this behavior (which could explain why the ecosystem "worked" without .bin collision).

I think it's fair to say that we're open to better alternatives, but prefer the default npm behavior (it'd certainly make our lives easier :D).

I'm staying subscribed to hear what the authors think. They rightfully have to be careful with introducing potentially-problematic functionality, but perhaps the quiet existence of this behavior in npm lessens the risk...

[Tapppi]

Is there a concise/reliable way to hoist the dependency bins? @frederickfogerty way seems quite fragile as he explained it. @bitfrost explained our use case, we do that too for company internal packages and it's a pain to add all the dependencies explicitly to all top level packages (never mind updates/maintenance when you are doing this across tens of packages).

[DanielZlotin]

Just wanted to share this https://github.com/wix/yarn-bin-fix. This is a temporary fix to our migration pains.

[frederickfogerty]

@DanielZlotin thanks for the tool! Do you personally list that in a postinstall step in your package.json, or do you run it manually?

[DanielZlotin]

As a post install script

On Nov 1, 2016 11:18 PM, "Frederick Fogerty" notifications@github.com
wrote:

@DanielZlotin https://github.com/DanielZlotin thanks for the tool! Do
you personally list that in a postinstall step in your package.json, or
do you run it manually?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1210 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGWgj3S3oC4p6pGjBG-2gRyV4Xbkag01ks5q56y9gaJpZM4KaUun
.

[bestander]

Looks like we have an issue with nested bin dependencies that collide with top level bin dependencies.
The solution should account on that.
Sending a PR to revert the change and skip the test.

[mateodelnorte]

Is the initial issue reported by this PR intended to be fixed? At this point, any number of plugin systems may be broken when installed via yarn because they expect child dependencies to be installed, as they are with npm.

[sethlesky]

I seem to be affected by a related issue.

yarn install will not install an expected sub package binary to this location during a Docker deployment with google-cloud or locally.
/app/node_modules/@google-cloud/datastore/node_modules/grpc/src/node/extension_binary/node-v48-linux-x64-glibc/grpc_node.node

Only after running npm install does the binary show up.

This breaks my google-cloud deployments when using yarn.

Apologies if this isn't the correct place to address this issue, any pointers would be appreciated though.

[Tapppi]

There seems to be some bug with some binaries, I'm sometimes hitting this with grpc and bcrypt binaries too. Usually a yarn install --force will fix it for me though. Never had this with a dockerised clean install though, only locally.