Feedback

[fisker]

No description provided.

[fisker]

It seems I have to use additional_dependencies

https://github.com/prettier/pre-commit/runs/953985278?check_suite_focus=true#step:8:24

[fisker]

I'm going AFK for a while.

[FloChehab]

It seems I have to use additional_dependencies

https://github.com/prettier/pre-commit/runs/953985278?check_suite_focus=true#step:8:24

In fact, it seems to be the way to go.
It's also how they proceed on for other mirrors: https://github.com/pre-commit/mirrors-eslint/blob/master/.pre-commit-hooks.yaml

[FloChehab]

I've tested it, and it works like a charm.

I've created a simple PR (#2) to make sure the versions in the different files don't go out of sync ; this will be helpful if you choose to go for dependabot instead of https://github.com/pre-commit/pre-commit-mirror-maker and this github action: https://github.com/pre-commit/mirrors-eslint/blob/master/.github/workflows/main.yml .

One thing to keep in mind, is that you will also need to keep manually creating tags when a new version is release ; something that seem to be also handled by the github action above.

For instance, the current commit on master can be / should be tagged 2.0.5 to be coherent with prettier's version.

Also for the tests failing on windows... I have no ideas ; I guess it's an error on pre-commit's side.

[fisker]

Thanks for testing, the reason I don't want additional_dependencies is I hope I can use dependabot to update prettier version, this is why I add it in package.json.

[fisker]

Thanks for the PR, but I still want Prettier version in just one place(package.json).

I have another idea, I'll try later.

[fisker]

@FloChehab What do you think the idea adding another bin file? #4

[FloChehab]

@FloChehab What do you think the idea adding another bin file? #4

Looks good to me ; and it works perfectly.

[fisker]

Thank you!

[FloChehab]

@fisker, Once you consider this repo done, could you create a tag v2.0.5 (or 2.0.5) to follow the version of prettier ?
I think it will be ok to "move this tag" afterwards, if we fix some bugs on the pre-commit support side.

[fisker]

I created a tag v1.0.0, do you think better follow prettier version?

What if we found bug, and pretter haven't release a new version?

[fisker]

The main reason I did't choose auto-update is some new version of Prettier might require changes, most likely we will ship --ignore-unknown in v2.1.0, we will need to update .pre-commit-hooks.yaml.

[fisker]

On a second thought, I might made mistake by using pretter in package.json instead of additional_dependencies.

If we use additional_dependencies, people should be able to override prettier version by addding additional_dependencies in .pre-commit-config.yaml.

But current solution I use should not able to do that.

I'm not sure, but I'll keep this in mind.

[FloChehab]

I created a tag v1.0.0, do you think better follow prettier version?

What if we found bug, and pretter haven't release a new version?

I think it will be ok to update the tag and make it point to another commit (black is doing this with their stable tag). As long as it's documented on the README. I would personally prefer having a consistent version number with prettier ; but why might want input from other people on this matter.

[FloChehab]

On a second thought, I might made mistake by using pretter in package.json instead of additional_dependencies.

If we use additional_dependencies, people should be able to override prettier version by addding additional_dependencies in .pre-commit-config.yaml.

But current solution I use should not able to do that.

I'm not sure, but I'll keep this in mind.

I thought it would be possible to override with additional_dependencies on the user side:

  - repo: https://github.com/prettier/pre-commit
    rev: "v1.0.0"
    hooks:
      - id: prettier
        args: ['--version']
        additional_dependencies: ["prettier@2.0.4"]

But it's not working (it shows 2.0.5).

So I guess, moving tags is ok.

[fisker]

Wait, you are tesing on v1.0.0, can you test this commit? before I remove additional_dependencies 44ca7e9

[FloChehab]

Wait, you are tesing on v1.0.0, can you test this commit? before I remove additional_dependencies 44ca7e9

Well, it works on this commit ! I do get 2.0.4 printed.

[FloChehab]

But on this commit, you don't have prettier version specified in https://github.com/prettier/pre-commit/blob/44ca7e9574378f14274107ab1c72d1eb1cced843/package.json
So no auto-update.

[fisker]

This commit has additional_dependencies

pre-commit/.pre-commit-hooks.yaml

Lines 20 to 21 in 44ca7e9

	additional_dependencies:
	- prettier@2.0.5

So I guess we still need use this property.

[FloChehab]

There is a choice to be made. I personally don't have a strong opinion on this matter.

[fisker]

@FloChehab Can it done by branch? we can create branch for every version.

[FloChehab]

Yes, pre-commit can work with any git reference, but my understanding is that it works best with tags (https://pre-commit.com/#pre-commit-autoupdate).

Maybe we can get the opinion of pre-commit's maintainer on what we are doing here (started here) ; as he is the one who added pre-commit support to prettier's main repo 3 years ago in prettier/prettier#2414 (@asottile I am trully sorry to bother you, but if you have a bit of spare time, please have a look at this.)

[fisker]

All branches https://github.com/prettier/pre-commit/branches and tags https://github.com/prettier/pre-commit/tags created.

[asottile]

Hi, didn't read the whole thread but I'm not sure this is the best thing to do here -- if you've set up caching it doesn't really make a difference and the extra repository is likely to lead to more confusion and indirection.

That said, I can help you get this set up if you want and describe some of the reasons / downsides for additional_dependencies.

the mirrors you see above were made while the project was in more of its infancy and upstreams were not as receptive to taking the pre-commit configuration (some of the reasoning for this was the unfortunate naming of the yaml file, that's at least been fixed). The mirror repositories aimed to have slightly more compatibility than your typical repo and used additional_dependencies to install things instead of utilizing a package.json (the original form of these did use package.json but npm did the whole peer dependency reshuffle thing and during that transition period it didn't really make sense to handle deps that way).

If you can find a way that works with package.json (and peer deps or whatever) that would be preferable to the additional_dependencies approach. The problem with additional_dependencies is when people want to add plugins they have to repeat whatever is upstream (this can often lead to version skew or other stuff). I haven't found a clever way to do this since the peer deps shuffle but then again I don't spend all that much time in the javascript community. If you can come up with a solution there, I'd like to also "steal" that and adapt it to the mirror repositories. For now, this was one of the advantages of not having a mirror repository, the usual use of additional_dependencies "just works" against a true non-mirror repo (that is, it doesn't clobber the actual file).

I think ideally if we could collaborate on those ideas there and get something working with package.json (even if it only supports modern npm) -- I'd be open to improvements in https://github.com/pre-commit/pre-commit-mirror-maker -- then you could reuse the automatic github action to keep this repository automatically updated.

I think the questions above were about pre-commit's preference for the rev field and I can hopefully clarify that by reiterating some of the documentation. pre-commit works best on immutable tags that proceed in a forward-only manner on the default branch. It uses these references to implement pre-commit autoupdate.

(unrelated but I saw it while scrolling through the other thread) The usage of language: fail seems pretty neat, I think I might reuse that for my deprecation usages -- I had not seen something like that before -- clever!

Lastly, once we're done with this, we'll want to update the reference to prettier on pre-commit.com -- that can be done with a PR to https://github.com/pre-commit/pre-commit.com

[fisker]

First of all, thanks for the quick reponse.

the extra repository is likely to lead to more confusion and indirection.

The main reason, we decide to use extra repo is, people will get unstable prettier from github. Npm (stable) version should prefered.

If you can find a way that works with package.json

I do found one solution to make it work with package.json, check https://github.com/prettier/pre-commit/pull/4/files , in this PR, I setup another bin file to call prettier.

But I decided to revert it, reason #1 (comment) .

I thought additional_dependencies is designed to override package versions(plugins, extra packages of course).

With the solution #4, people won't be able to override Prettier.

[asottile]

Usually they'll control the prettier version through the rev field of their pre-commit configuration -- I think maybe peer_dependencies in package.json would put the prettier binary in the right place without any special wrapper tool? I'm not sure though and would have to play with it

[fisker]

then you could reuse the automatic github action to keep this repository automatically updated.

I still prefer not auto-update because new version of Prettier might require changes, for example v2.1.0 will ship ignore-unknown, .pre-commit-hooks.yaml will remove files field, and add the new args

[asottile]

makes sense!

[fisker]

Usually they'll control the prettier version through the rev field of their pre-commit configuration -- I think maybe peer_dependencies in package.json would put the prettier binary in the right place without any special wrapper tool? I'm not sure though and would have to play with it

No, peer_dependencies won't install at all, it need install by yourself.

[fisker]

@asottile As I understand, pre-commit do npm i -g . , so if prettier is in package.json dependencies, it will be installed in node_modules/.bin, so , is that possibe to search this place to find it? Something like add globalDir and globalDir/prettier-for-pre-commit/node_modules/.bin to PATH and other packages too.

[asottile]

it isn't currently, the only place that pre-commit currently searches is the {env}/bin directory

in older versions of npm it was possible to trigger an install -g as a side-effect of installation and that was how it was triggered pre-commit/pre-commit-mirror-maker@300bbd4#diff-2be39c1f51329de4c0e0cf56648acbabL6 -- I forget what changed there but something made this approach no longer work

[fisker]

Is this {env}/bin real npm prefix dir? Or just for pre-commit

[asottile]

it's the true npm prefix dir, it's set up as an isolated environment using nodeenv

[fisker]

Wow, I thought that was only for pre-commit. No offence, but I think this is a bad idea, tools like ESLint not work well with global installation and "not recommended" https://eslint.org/docs/user-guide/getting-started#installation-and-usage, Prettier don't like global installation too prettier/prettier#8637 .

[fisker]

This also means, if I use the one from prettier/prettier repo, my global Prettier from npm will be replaced.

[fisker]

Understanding this makes me think #4 is actually a good solution for now, it won't override global one, Prettier will be a sub dependeny of prettier-pre-commit package.

[asottile]

no no, that's not how it works -- it makes entirely isolated environments :)

[fisker]

no no, that's not how it works -- it makes entirely isolated environments :)

This make sense.

[fisker]

What do you think the idea of searching more places? That's how yarn/npm run/npx works.

[asottile]

you mean like node_modules/.bin as well as {env}/bin -- I'm not sure what that would break (or which should take precendence) -- historically the .bin has been thrown away in the pre-commit environments 🤔

[fisker]

Yes, that's what I mean.

[FloChehab]

Just two side notes after reading your discussion:

• Thanks a lot for your input @asottile ; I am not an npm / node / etc. expert ; but I am enjoying a lot both tools (prettier / pre-commit) and I have some spare time, so if you need any help for testing or a bit of development, I'll be very happy to contribute.

• I was thinking the other day that it could be great if pre-commit supported installing "hooks" directly from npm or pypi (besides git); which could resolve the current issues with broken dependencies, clone / build time (?)

[FloChehab]

Hi @fisker, sorry I forgot to come back to this issue. So should we consider the current setup as final ?

[fisker]

I think so.