pre-commit hook raising an error about missing ./dist/pyright

[brettcannon]

Describe the bug

pyright..................................................................Failed
- hook id: pyright
- exit code: 1

internal/modules/cjs/loader.js:1033
  throw err;
  ^

Error: Cannot find module './dist/pyright'
Require stack:
- /Users/brettcannon/.cache/pre-commit/repopxecb0be/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1030:15)
    at Function.Module._load (internal/modules/cjs/loader.js:899:27)
    at Module.require (internal/modules/cjs/loader.js:1090:19)
    at require (internal/modules/cjs/helpers.js:75:18)
    at Object.<anonymous> (/Users/brettcannon/.cache/pre-commit/repopxecb0be/index.js:6:1)
    at Module._compile (internal/modules/cjs/loader.js:1201:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1221:10)
    at Module.load (internal/modules/cjs/loader.js:1050:32)
    at Function.Module._load (internal/modules/cjs/loader.js:938:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/brettcannon/.cache/pre-commit/repopxecb0be/index.js' ]
}

To Reproduce
Set up pyright in pre-commit:

 - repo: https://github.com/microsoft/pyright
   rev: 1.1.53
   hooks:
   - id: pyright

[bschnurr]

'./dist/pyright' gets built by running "npm run package"

https://github.com/microsoft/pyright/blob/master/docs/build-debug.md

[erictraut]

@brettcannon, the hook assumes that the Pyright CLI has been installed via npm. Have you done that? Alternatively, you could build the full cli using "npm run package", as Bill mentioned.

[brettcannon]

@erictraut no as pre-commit is often used to isolate a project from having to install individual tools (e.g. just run pre-commit in CI and have everything just work), and specifically in pyright's case I was hoping I could avoid doing anything directly npm-related.

[jakebailey]

The pre-commit hook appears to declare pyright as using the python language (technically true). Does the config need to include more steps / more dependencies to make it work? I'm unfamiliar with pre-commit.

Skimming some other hooks, they declare that they need node, for example: https://github.com/elidupuis/mirrors-sass-lint/blob/master/.pre-commit-hooks.yaml

So I'm thinking that the hook needs to be expanded to a more functional state if it's to be used.

[brettcannon]

I think I may have figured out how this is expected to be done, but I need to test it first.

[brettcannon]

OK, my idea didn't work.

It looks like pre-commit runs npm install . but I don't see a call to npm build. https://pre-commit.com/#node

[erictraut]

Why do you want to build pyright from source? It's published in npm under the name "pyright", so "npm install pyright" should install it.

[jakebailey]

The intent is that pre-commit abstracts away the idea of a CI tool, and you just ask to run some tool (and the repo's .pre-commit-hooks.yaml file defines that for the tool). That's what I believe #708 was intended to do.

Perhaps @nihaals can comment on how they got it to work.

[nihaals]

Comparing to a mirror isn't accurate as they don't actually have the source, all they do is bump the external dependency. A more accurate comparison is Prettier

Can repro, looks like it only runs npm install ., which doesn't add the pyright command. Some possible workarounds for now:

1. Use the same method as mirrors
2. Use a local repository then use something like npx to use the installed tool

Possibly useful repro steps:

$ docker run -it --rm orangutan/playground:alpine
# apk add python3 py3-pip nodejs npm
# pip3 install --user -U pre-commit
# mkdir /app
# cd /app
# git init
# touch main.py
# cat <<EOF > .pre-commit-config.yaml
repos:
  - repo: https://github.com/microsoft/pyright
    rev: 1.1.54
    hooks:
      - id: pyright
EOF
# git add .
# ~/.local/bin/pre-commit

[jakebailey]

FWIW I wouldn't expect this to work as-is in this repo. pyright is TS and the code has to be compiled first. Prettier is all in JS, and their "binary" just runs that code, so they avoid the problem altogether.

If you run the distributed version (npm install pyright or npx pyright), you're getting the published copy after the build has completed (i.e., the cli webpack).

I did some snooping, and there is a pre-commit hook for tslint someone made (tslint is of course in TS), but it's essentially an empty repo: https://github.com/awebdeveloper/pre-commit-tslint

I'm struggling to find a TS project that declares itself a pre-commit hook, only what I think you meant by mirrors (like https://github.com/pre-commit/mirrors-coffeelint/blob/master/.pre-commit-hooks.yaml which just says "run coffescript at this version").

[nihaals]

I believe when I tested my PR I may have used an additional dependency which behaves differently when npm install . isn't the same as npm install <package>. Having a mirror in the main repo wouldn't be great as if you wanted to use it, it would clone the entire repo just for the single file, and will run npm install . even though it isn't used. Having something like microsoft/pyright-pre-commit or mirror wouldn't make much sense either, so I guess the best option is an "unofficial" mirror unless there's another way to do this (for example stuffing more commands into the entry field e.g. npm run build && pyright). A script or system hook also seems viable, not sure about having it work with Bash and Windows, pre-commit may already have something built in to handle running multiple commands on any system. There could be a hook that's already implemented something very similar with these hook types

@asottile is there a good approach for this? There doesn't seem to be any docs on it

[asottile]

this looks like something that pre-commit fixed ~relatively recently -- 1.15.0? -- essentially pre-commit installs similar to npm install git+...

this was changed at some point so that prettier works properly, you may be able to take a peek at their packaging? https://github.com/prettier/prettier/blob/master/package.json

[asottile]

iirc the npm lifecycle event is prepare or something similar

[jakebailey]

Prettier is entirely in JS. The binary executed by the hook is just: https://github.com/prettier/prettier/blob/master/bin/prettier.js

So cloning the repo and installing deps has an immediately executable setup because it can just run the source directly.

Pyright is different in that it's written in TypeScript, so the repo contains TypeScript code that needs to be compiled before it can execute, and the distributed code on npmjs is actually coming out of webpack during the publish step, so it's not purely the same as running the compiled TypeScript output either. This repo's also unique given that it has a few nested packages to support different setups (running as a VS Code extension, CLI tool, and server), and npm install at the top level doesn't fully install the deps.

@asottile I believe you are the/a pre-commit maintainer (hello!). I think for node projects it can make more sense for them to be installed off of npmjs directly, or run with npx <name>. Is there any method by which a consumer of a hook could just say "I want this node package at this version"? Or does that pose issues because npmjs needs to somehow store the hook metadata (how to run, which file types, etc).

[asottile]

pyright currently can't be installed via npm install git+https://github.com/microsoft/pyright -- that's the core problem and is unrelated to pre-commit

[jakebailey]

Yeah, that's unfortunately not possible at the moment due the aforementioned folder structure and build setup. If we switch to a system like rush to manage things, this might improve (big maybe), but that won't change the fact that this repo has to contain multiple packages. Not sure what npm's method of resolution is for mono-repo style projects where things are in subdirectories. I know react-scripts, eslint, etc, are using this style (and I would expect them to be installable via that method).

[asottile]

fwiw, this patch fixes:

diff --git a/package.json b/package.json
index b4de11c3..d1f15922 100644
--- a/package.json
+++ b/package.json
@@ -24,6 +24,7 @@
         "build:serverDebug": "cd server && npm run build:serverDebug && cd ..",
         "build:cli": "cd server && npm run build:cli && cd ..",
         "clean": "del-cli \"./client/server\" && del-cli \"./client/out\" && del-cli \"./dist\"",
+        "prepare": "cd server && npm i && cd .. && npm run build:serverProd && npm run build:cli",
         "package": "npm run install:all && npm run clean && npm run build:serverProd && npm run build:cli && cd client && npx vsce package && cd .."
     },
     "devDependencies": {

#!/usr/bin/env bash
set -exo pipefail

git clean -fxfd

~/opt/venv/bin/nodeenv nenv --prebuilt --clean-src
. nenv/bin/activate
npm install
npm install -g .

pyright --help

$ bash ../t.sh

...

+ npm install -g .
/tmp/pyright/nenv/bin/pyright -> /tmp/pyright/nenv/lib/node_modules/pyright/index.js
/tmp/pyright/nenv/bin/pyright-langserver -> /tmp/pyright/nenv/lib/node_modules/pyright/langserver.index.js
+ pyright@1.1.54
added 1 package from 1 contributor in 1.042s
+ pyright --help
Usage: pyright [options] files...
  Options:
  --createstub IMPORT              Create type stub file(s) for import
  --dependencies                   Emit import dependency information
  -h,--help                        Show this help message
  --lib                            Use library code to infer types when stubs are missing
  --outputjson                     Output results in JSON format
  -p,--project FILE OR DIRECTORY   Use the configuration file at this location
  --stats                          Print detailed performance stats
  -t,--typeshed-path DIRECTORY     Use typeshed type stubs at this location
  -v,--venv-path DIRECTORY         Directory that contains virtual environments
  --verbose                        Emit verbose diagnostics
  --version                        Print Pyright version
  -w,--watch                       Continue to run and watch for changes

[jakebailey]

This was also pointed out to me, that a mirror already exists to run pyright off of npm: https://github.com/necaris/pre-commit-pyright

(though clearly a release here would not immediately bump that mirror)

[erictraut]

That mirror's hook file includes:

additional_dependencies: ["pyright@1.1.53"]

Would it suffice to add that to the .pre-commit-hooks.yaml file that's checked in to the pyright repo? I could them bump the version each time I publish.

[nihaals]

Having a mirror in the main repo wouldn't be great as if you wanted to use it, it would clone the entire repo just for the single file, and will run npm install . even though it isn't used.

Adjusting the package.json is probably the best way

[jakebailey]

As mentioned in #862 (comment), this would mean that pre-commit clones this repo, runs npm install, then immediately installs and runs another package entirely, which might be a bit hefty...

I'm still on the fence about modifying package.json to build everything when you install, though. I've been looking around at other TS packages and it doesn't appear all that consistent. But I'm also seeing that many don't webpack, instead shipping the tsc output.

[erictraut]

Yeah, I don't think users should need to build pyright to use it as a hook. Is there a way to simply specify that the latest version published in npmjs.org should be used? https://www.npmjs.com/package/pyright

[jakebailey]

Maybe if pre-commit had some sort of no_install: true option to disable the behavior (so that it could be fetched from npm) or some generic control over the commands run after checkout, but I'm thinking a distinct mirror repo that is maintained may be more successful given the performance implications of cloning the whole repo and building it (at least until the build is optimized).

[erictraut]

I don't have any interest in maintaining a separate mirror repo. I think for now I'll just delete the ".pre-commit-hooks.yaml" since it appears to be broken.

[nihaals]

Definitely agree having an official mirror repository is silly

Maybe this issue could stay open and be renamed or there could be a note for the future so if there becomes a nicer way to install in the future it can be re-implemented

[erictraut]

I don't like to leave issues open. Feel free to re-open in the future or create a new feature request if there's a better method identified.

[nihaals]

Side note, what was the issue with adding the prepare line? Could be useful if people want to install from git for whatever reason or just to use the standard NPM way of installing from source. Even if it's hardly used, it appears to be the best way to define some kind of standard build method

[erictraut]

Downloading all of the dev dependencies and building pyright from scratch can't possibly be the right answer.

[nihaals]

I'm saying on another note, not even to necessarily solve this issue. If there's a standard field then why not try to use it I guess

[jakebailey]

@brettcannon I know you opened this trying to make pre-commit work, but then the file got removed entirely which is a bit in the opposite direction of what you'd need...

I think you can define a hook in your repo as a local hook to run pyright, something like:

-   repo: local
    hooks:
    -   id: pyright
        name: pyright
        entry: pyright
        language: node
        pass_filenames: false
        types: [python]
        additional_dependencies: ['pyright@1.1.55']

Does this work?

[brettcannon]

@jakebailey It appears to: https://github.com/brettcannon/python-project-template/pull/19/files.

I'll keep this around for now until https://pre-commit.ci/ exists, at which point I will switch to the mirror for automatic updates. Any plans to document this and/or the mirror somewhere for people to know?

[erictraut]

That's great to hear. Yeah, I think it's worth adding a documentation page. I can take a crack at that and have you review. Thanks!

[erictraut]

I've added a docs page that includes some instructions for integrating pyright in to a CI system.

https://github.com/microsoft/pyright/blob/master/docs/ci-integration.md

Input is welcome!

[nihaals]

Great docs, only thing I would change is being more specific that you are talking about pre-commit, maybe calling it a pre-commit hook instead of just git and include a link so users who haven't seen it know what it is

Pretty minor but for people who are super new they might also benefit from an example GitHub Actions workflow (pre-commit's already got an action and docs but people might find a setup-node, npm install, pyright workflow more useful)

[erictraut]

Thanks @nihaals. I've updated it to call it a "pre-commit hook". I don't know enough about GitHub actions workflow to write details docs here. Feel free to submit a PR if you want.