Suggestion: move pre-commit support to another repo

[FloChehab]

Hi,
First of all, thanks for the awesome tool, I have been using for a couple of years and have never been disappointed ! :)

This issue is more a of a question / suggestion, so sorry if it doesn't fit in the standard issue schema.

Context
I am currently using prettier to format files not related to JavaScript through the https://pre-commit.com/ integration. Because (from what I understand) pre-commit runs npm i . it's pretty slow and bandwidth intensive on this repo (takes around 40-50s to set up in our CI environment).
The other issue is that sometimes, sub-dependencies of prettier are broken (like today, see the error below - v2.0.5).

By looking at the npm page I see that prettier is actually a standalone package: https://www.npmjs.com/package/prettier ; and doing npm i [email protected] takes actually less than 1 second, which is pretty great 🎉 .

So here is my suggestion:
Could it be possible to move the 'official' support of pre-commit to another git repo, that would have a simple package.json with only prettier and the same hook file as https://github.com/prettier/prettier/blob/master/.pre-commit-hooks.yaml ?

This would

• Be faster to install (which is also good for the 🌍 ),
• Avoid deps errors,
• Require the fairly small maintenance of another repo.

I have currently switched to https://github.com/awebdeveloper/pre-commit-prettier which is kind of what I am suggesting, but without the native tag support.

Have a nice day !

---

Sub dependancies errors

An unexpected error has occurred: CalledProcessError: command: ('/home/jenkins/.cache/pre-commit/repole0tq7th/node_env-default/bin/node', '/home/jenkins/.cache/pre-commit/repole0tq7th/node_env-default/bin/npm', 'install')

return code: 1

expected return code: 0

stdout: (none)

stderr:

    npm WARN deprecated [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-babel.

    npm WARN deprecated [email protected]: `json-parser` is deprecated. Please use `comment-json` instead

    npm WARN deprecated [email protected]: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.

    npm WARN deprecated [email protected]: Please see https://github.com/lydell/urix#deprecated

    npm WARN deprecated [email protected]: https://github.com/lydell/resolve-url#deprecated

    npm WARN deprecated [email protected]: fsevents 1 will break on node v14+ and could be using insecure binaries. Upgrade to fsevents 2.

    npm WARN deprecated [email protected]: request-promise-native has been deprecated because it extends the now deprecated request package, see https://github.com/request/request/issues/3142

    npm WARN deprecated [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142

    npm WARN deprecated [email protected]: this library is no longer supported

    npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^2.1.2 (node_modules/jest-haste-map/node_modules/fsevents):

    npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

    npm WARN notsup Unsupported engine for [email protected]: wanted: {"node":"<8.10.0"} (current: {"node":"14.7.0","npm":"6.14.7"})

    npm WARN notsup Not compatible with your version of node/npm: [email protected]

    npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@^1.2.7 (node_modules/watchpack-chokidar2/node_modules/chokidar/node_modules/fsevents):

    npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

    npm WARN @rollup/[email protected] requires a peer of rollup@^1.20.0 but none is installed. You must install peer dependencies yourself.

    npm WARN @rollup/[email protected] requires a peer of rollup@^1.20.0 but none is installed. You must install peer dependencies yourself.

    npm WARN @rollup/[email protected] requires a peer of rollup@^1.20.0 but none is installed. You must install peer dependencies yourself.

    npm WARN @rollup/[email protected] requires a peer of rollup@^1.20.0 but none is installed. You must install peer dependencies yourself.

    npm WARN @rollup/[email protected] requires a peer of rollup@^1.20.0 but none is installed. You must install peer dependencies yourself.

    

    npm ERR! code E404

    npm ERR! 404 Not Found - GET https://registry.npmjs.org/@rollup/plugin-replace/-/plugin-replace-2.3.1.tgz

    npm ERR! 404 

    npm ERR! 404  '@rollup/[email protected]' is not in the npm registry.

    npm ERR! 404 You should bug the author to publish it (or use the name yourself!)

    npm ERR! 404 It was specified as a dependency of 'repole0tq7th'

    npm ERR! 404 

    npm ERR! 404 Note that you can also install from a

    npm ERR! 404 tarball, folder, http url, or git url.

    

    npm ERR! A complete log of this run can be found in:

    npm ERR!     /home/jenkins/.npm/_logs/2020-08-06T09_02_33_456Z-debug.log

[alexander-akait]

We don't have pre-commit in this repo. How do you install prettier? We bundle prettier and when you run npm i prettier, no extra dependencies installed, using prettier from master branch is not a safe

[alexander-akait]

I think you have something wrong with your pre-commit configuration

[FloChehab]

You do have support for pre-commit through this file: https://github.com/prettier/prettier/blob/master/.pre-commit-hooks.yaml

[fisker]

I think this a good idea.

[fisker]

Maybe we can just import https://github.com/awebdeveloper/pre-commit-prettier to prettier/pre-commit

[FloChehab]

Maybe we can just import https://github.com/awebdeveloper/pre-commit-prettier to prettier/pre-commit

I think https://github.com/awebdeveloper/pre-commit-prettier setup is not ideal because the version handling is not "native".

You have to do:

  - repo: https://github.com/awebdeveloper/pre-commit-prettier
    rev: "master"
    hooks:
      - id: prettier
        additional_dependencies: ["[email protected]"]

What I am suggesting is a repo that follows the main repo in terms of tags / versions (at least for the releases to npm).

So that we could do:

  - repo: https://github.com/prettier/pre-commit
    rev: "2.0.5"
    hooks:
      - id: prettier

I can work on the repo (under a day, it should be pretty simple), and you shall decide if you integrate it in the prettier organization (I wouldn't want to take ownership of it personally).

[alexander-akait]

Just clarify, does pre-commit get https://github.com/prettier/prettier/blob/master/.pre-commit-hooks.yaml file automatically on installation?

[fisker]

@evilebottnawi check this https://prettier.io/docs/en/precommit.html#option-3-pre-commithttpsgithubcompre-commitpre-commit

[alexander-akait]

oh, it is weird, why do not use packages npm register? It is not working with pre-commit?

[fisker]

@FloChehab One off topic question, do you know is this change safe? Is it okay to remove files? Will it run on every file? None of us use pre-commit.

[fisker]

oh, it is weird, why do not use packages npm register? It is not working with pre-commit?

I think I found something https://pre-commit.com/#node

[alexander-akait]

@fisker yep, I think we need to rewrite docs, because using master is really bad idea, it can be unstable between commits

[FloChehab]

oh, it is weird, why do not use packages npm register? It is not working with pre-commit?

Unfortunately this is not the way pre-commit works, I am definitely not an expert of it, but from what I understand, it's only git based (which is pretty great for easy integration). I don't know if there is any plan on expanding the support to npm (or pip) directly.

[FloChehab]

@FloChehab One off topic question, do you know is this change safe? Is it okay to remove files? Will it run on every file? None of us use pre-commit.

@fisker I'll have a look tonight ! :)

[fisker]

@fisker I'll have a look tonight ! :)

Great appreciate!

[FloChehab]

@fisker yep, I think we need to rewrite docs, because using master is really bad idea, it can be unstable between commits

Also, the main point of this issue, is actually that "old" tags are also unstable for pre-commit.

[fisker]

I wonder if this the main reason we support install-from-github.

[fisker]

Seems there is a tool for this https://github.com/pre-commit/pre-commit-mirror-maker

[FloChehab]

Seems there is a tool for this https://github.com/pre-commit/pre-commit-mirror-maker

Nice ; so it should be even easier. At the end, I think the final repo should look like https://github.com/pre-commit/mirrors-jshint .

[FloChehab]

So there might be discussions regarding under which github organization it might end up.

[fisker]

I prefer it under @prettier, @evilebottnawi what do you think?

[fisker]

I've created https://github.com/prettier/pre-commit

[alexander-akait]

make sense

[FloChehab]

Thanks @fisker & @evilebottnawi for the fast action / decision making !

I'll test the new repo setup in few hours ; and I'll make feedbacks there.

[fisker]

Let's keep this open, until prettier/pre-commit is stable and we can remove .pre-commit-hooks.yaml and update docs.

Let's move discuss to prettier/pre-commit#1

[fisker]

I think I've done setting up https://github.com/prettier/pre-commit.

How do we notify people migrate to the new repo?

Something like this?

- id: prettier
  name: prettier
  entry: echo prettier support for pre-commit have been moved to https://github.com/prettier/pre-commit, please checkout the new repository. && exit 1

[FloChehab]

You can go for:

      - id: prettier
        name: prettier
        entry: Prettier support for pre-commit have been moved to https://github.com/prettier/pre-commit, please checkout the new repository.
        language: fail
        pass_filenames: false

EDIT: added pass_filenames: false

[fisker]

Thank you, I'll try.

[fisker]

Great, it works, I tested on CI https://github.com/prettier/pre-commit/pull/6/checks?check_run_id=957631819#step:7:21

Do you want send a PR for this?

---

.pre-commit-hooks.yaml I used for test

[FloChehab]

Sure, I'll do it ; we will need to also update prettier's documentation.

[fisker]

Thanks!