Closed stefanpenner closed 5 years ago
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.
Added a big comment on the Autoshim RFC about this issue: https://github.com/notion-cli/rfcs/pull/23#issuecomment-453300567
@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:
@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.
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.
@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.
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.
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 likesyarn
ornpm
viapackage.json#scripts
or directly vianpx
.So for example given:
invoking
yarn test
would result in:•
yarn
being provided bynotion
mocha
invoked byyarn test
being provided byyarn
's prepending of./node_modules/.bin
to $PATHinvoking
mocha
directly in the shell would consult the$PATH
but not encounter any relevantnotion
shims, that is unless a user opted in to a globalmocha
provided bynotion
.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 shadowwhich
, with a strange node basedwhich
Some highlights:
Additional risk:
Nothing prevents a malicious user from taking advantage of this behavior, and replace an important executable such as
sudo
orsu
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.