remove autoshimming

[stefanpenner]

Edit by @dherman: This issue kicked off a super useful set of design discussions. We'll use it to remove autoshimming. Implementing this is blocked on #148. (See below.)

---

Notion automatically exposing all of ./node_modules/bin/* to my $PATH is quite surprising to me and I believe it may pose some serious risks.

Expectations

Until now I was under the impression that notion was aiming to solve the "global" problem, and leave the ./node_modules/.bin executables to the likes yarn or npm via package.json#scripts or directly via npx.

So for example given:

// package.json
{
  ...
  "scripts": {
    "test": "mocha test"
  }
  ...
}

invoking yarn test would result in:

• yarn being provided by notion

• mocha invoked by yarn test being provided by yarn's prepending of ./node_modules/.bin to $PATH

invoking mocha directly in the shell would consult the $PATH but not encounter any relevant notion shims, that is unless a user opted in to a global mocha provided by notion.

Concerns with existing approach

On my machine, I would have hundreds unexpected executables on my $PATH.

examples which executables which would be exposed on my machine

• JSONStream
• _mocha
• _ts-node
• acorn
• analyze-css
• ansi-html
• ansi-to-html
• artdeco-upgrade
• atob
• autoprefixer-info
• ava
• await-outside
• babel
• babel-doctor
• babel-external-helpers
• babel-node
• babel-plugin
• babylon
• bench
• bower
• broccoli
• broccoli-concat-analyser
• broccoli-module-alchemist-install
• broccoli-timepiece
• browser-pack
• browser-sync
• browserify
• browserslist
• buble
• build-template
• c8
• cake
• can-symlink
• cat-names
• cdl
• chromedriver
• cleancss
• codeclimate-test-reporter
• codecov
• codemod-cli
• coffee
• coffeelint
• color-support
• colorguard
• commitlint
• commitlint-travis
• commitplease
• commonize
• concurrent
• concurrently
• conventional-changelog
• conventional-changelog-writer
• conventional-commits-parser
• conventional-recommended-bump
• coveralls
• cpr
• cross-env
• cross-env-shell
• css-beautify
• css-rule-stream
• cssesc
• cssmin
• csso
• csv2json
• csv2tsv
• cypress
• datauri
• dateformat
• decompress-zip
• defs
• deps-sort
• derequire
• detect
• detect-indent
• detect-libc
• detect-port
• detective
• dev-ip
• direction
• disparity
• docco
• documentation
• doiuse
• dsv2dsv
• dsv2json
• dustc
• eclint
• ecmarkup
• ecstatic
• editorconfig
• ember
• ember-cli-sauce
• ember-cli-update
• ember-modules-codemod
• ember-source-channel-url
• ember-template-lint
• ember-test-helpers-codemod
• ember-watson
• envinfo
• errno
• escodegen
• esgenerate
• eslint
• eslint-config-prettier-check
• esparse
• esperanto
• esvalidate
• extract-zip
• faucet
• find-project-root
• findup
• firefox-profile
• flow
• fmd
• get-pkg-repo
• git-diff-apply
• git-raw-commits
• git-semver-tags
• gitbook
• glob-all
• gnode
• gonzales
• google-chrome
• grunt
• grunt-compare-size
• grunt-istanbul
• gulp
• gunzip-js
• gunzip-maybe
• gzip-js
• handlebars
• har-validator
• has-ansi
• he
• highlight
• hs
• html-beautify
• html-differ
• html-minifier
• http-server
• import-local-fixture
• in-install
• in-publish
• inline-source-map-comment
• insert-module-globals
• internal-ip
• is-ci
• istanbul
• jade
• jake
• jest
• jest-runtime
• jison
• js-beautify
• js-yaml
• jscodeshift
• jscs
• jsdoc
• jsesc
• jshint
• jsmin
• json2csv
• json2dsv
• json2ts
• json2tsv
• json2yaml
• json5
• jsonapi-validator
• jsonfilter
• jsonlint
• jstf-testem
• juice
• karma
• lantern
• latest-version
• lerna
• lerna-changelog
• leven
• li-wdio
• lightercollective
• lint-staged
• lint-validator
• livereload
• loose-envify
• lorem-ipsum
• lt
• lz-string
• markdown-it
• markdown-toc
• marked
• markup-lint
• markup-toDraft
• markup-toHTML
• markup-toJSON
• markup-toMarkdown
• markup-toText
• md2html
• metalsmith
• miller-rabin
• mime
• mimer
• mkdirp
• mocha
• mocha-esm
• mocha-only-detector
• mocha-only-detector-glob
• mocha-typescript-watch
• module-deps
• multicast-dns
• multidep
• mustache
• ncp
• nearley-railroad
• nearley-test
• nearley-unparse
• nearleyc
• needle
• node-gyp
• node-mp-bridge
• node-pre-gyp
• node-sass
• nodemon
• nomdown
• nomnoml
• nopt
• normalize-pkg
• not-in-install
• not-in-publish
• notify
• npm
• npm-path
• npm-which
• npx
• nunjucks-precompile
• nyc
• opener
• opn
• os-name
• osx-release
• pad
• parse-github-url
• parser
• pemberly-core
• perfectionist
• perfology
• phantomjs
• pkcs7
• play-npm-build
• play-npm-clean
• play-npm-start
• play-npm-test
• play-npm-upgrade
• prettier
• pretty-ms
• project-name
• promises-aplus-tests
• qrcode-terminal
• qunit
• r.js
• r_js
• rb-status
• rc
• regenerator
• regexpu
• regjsparser
• remap-istanbul
• remarkable
• repeating
• rimraf
• rollup
• rtlcss
• safe-publish-latest
• sane
• sass
• sass-lint
• sass-lint-auto-fix
• sassdoc
• sassgraph
• saucie
• seek-bunzip
• seek-table
• selenium-standalone
• semantic-release
• semver
• sha.js
• shjs
• showdown
• sl-log-transformer
• sort-package-json
• specificity
• sshpk-conv
• sshpk-sign
• sshpk-verify
• start-selenium
• static
• strip-ansi
• strip-bom
• strip-indent
• strip-json-comments
• stripify
• stylehacks
• stylelint
• stylus
• supports-color
• svgo
• sw-precache
• swiffer
• swig
• tacks
• tap
• tap-mocha-reporter
• tap-parser
• tape
• terser
• testem
• throttleproxy
• to-title-case
• touch
• travis-deploy-once
• tree-kill
• ts-docs-gen
• ts-node
• tsc
• tslint
• tslint-config-prettier-check
• tsr
• tsserver
• tsv2csv
• tsv2json
• typedoc
• uglifyjs
• umd
• undeclared-identifiers
• user-home
• username
• uuid
• validate-template
• vweb-ember-qunit-codemod
• watch
• watchify
• wd
• wdio
• webpack
• webpack-cli
• webpack-dev-server
• weinre
• which
• window-size
• xml-js
• xo
• yaml2json
• yuidoc
• z-schema
• 🐹

Unexpected shadowing

By auto exposing all executables provided by a dependency, a user may be plagued by unexpected shadowing of executables. For example, a node project with node-which as a dependency, would now unexpectedly shadow which, with a strange node based which

Some highlights:

• google-chrome (this would confuse testem, as it checks for existence)
• which (this is too meta for me to reason about )

Additional risk:

Nothing prevents a malicious user from taking advantage of this behavior, and replace an important executable such as sudo or su or ....

Increased Risk of confusing which

Build tools and shells scripts commonly rely on which <name-of-thing> to select specific behavior. With hundreds of potentially broken detections, this risk of "heisenbug" issues arrises increases dramatically.

Potential Unbounded growth of these executables

As a user uses more and more node projects, the number of these executables increase, without an obvious way to garbage collect

Recommendation

Transition from automatically installing all ./node_module/.bin, and rather allow users to opt-in via configuration.

[charlespierce]

I’m definitely in favor of not autoshimming the dependency binaries. I don’t think I’ve seen a project where a user installing a node dependency expects the binaries to be available in a general shell.

We could potentially document the shim command and make it the official way for users to “opt in” to having notion manage commands.

[charlespierce]

Added a big comment on the Autoshim RFC about this issue: volta-cli/rfcs#23 (comment)

[dherman]

@stefanpenner I think @charlespierce really nailed the core mental-model mistake I was making in the when I first led us down the path of auto-shimming. The problem I thought we were solving was that it seemed like we had a hole in the core Notion abstraction. But as @charlespierce pointed out, there's just never an expectation with Node that package executables will be in your PATH except when called from npm scripts (which already works fine in Notion). So even setting aside your critiques, it was just solving a non-problem.

Meanwhile, I also didn't like the idea that the only way to create shims would be for project maintainers to add them to some opt-in list in the manifest.

But I really like the idea that shims should be tied to notion install / notion uninstall. This really feels like the most coherent model:

• Users are in control of what tools they install or uninstall.
• There's just one toolchain: the set of tools the user has installed. Easier to understand than "user toolchains" vs "project toolchains."
• The semantics of invoking a tool in your toolchain is, as always, that it uses the version you installed by default, but if you're in a project that depends on it, it uses the project's selected version.

@charlespierce I still think the word "shim" is somewhat of a implementation-layer concern that I'm reluctant to expose in normal user commands. What do you think of the design I sketched above? I'll work on revising the toolchains RFC to spell it out in a little more detail.

[stefanpenner]

there's just never an expectation with Node that package executables will be in your PATH except when called from npm scripts (which already works fine in Notion).

Yup, exactly.

[charlespierce]

@dherman Yeah, I like the approach of conceptually having a single toolchain, and having it managed by the notion install and notion uninstall commands. That fits perfectly into the mental model that people already have of installing / uninstalling tools, and is an easy mapping to do: Instead of npm i -g <TOOL>, you do notion install <TOOL> and it's the same effect, except that we provide the local version guarantees in projects that have a specific version of a tool.

[dherman]

This issue kicked off some fantastic design discussions. I think we've landed on a much better design, which is not only safer (in re: the concerns raised by this issue) but should be easier to understand, learn, and manage. You can see the latest iteration of the design in the toolchain RFC.

I've renamed this issue to repurpose it to remove the autoshimming functionality, which shouldn't be too hard to implement.

Important: Let's not implement this until we've finished implementing notion install <package>. IOW, autoshimming can pragmatically serve as our transitional, if imperfect, mechanism for installing shims.