WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
233 stars 183 forks source link

Prettier's pre-commit plugin is no longer maintained #4256

Closed sarayourfriend closed 3 weeks ago

sarayourfriend commented 4 months ago

Current Situation

See the note here: https://github.com/pre-commit/mirrors-prettier?tab=readme-ov-file#archived

Suggested Improvement

Find a new way to run prettier on pre-commit (probably locally?)

Benefit

Not using an unmaintained piece of software

Additional context

dhruvkb commented 4 months ago

Seeing pre-commit's maintainer's general comments across GitHub I don't think we'll even get an explanation beyond "prettier made some changes that breaks plugins entirely".

sarayourfriend commented 3 months ago

Maybe worth considering a change to https://github.com/typicode/husky and https://github.com/lint-staged/lint-staged instead of pre-commit.

If we need to run prettier locally anyway, pnpm (and therefore node) is suddenly a hard and fast dependency (because prettier is required for documentation linting). Switching to lint-staged would require some up-front time to convert our checks, however, it would subsequently simplify our management of the tool by removing the need to install a separate tool (and manage that tool using unique means, i.e., downloading the .pyz). husky and lint-staged would just be a monorepo-level package.json dependency, and only a pnpm install away from being configured. That simplifies our CI scripts too, no need to manage pre-commit there either.

The best part of pre-commit are the isolated environments for each hook, and we can certainly do that ourselves if we like. But combined with a PDM dependencies configuration at the root pyproject.toml, we could just as easily run all the tools we use in the monorepo with either pnpm run, docker run, or pdm run. We can use docker run anytime we really do need an isolated environment rather than running directly in the repository.

@dhruvkb as our repository devex tooling aficionado... what do you think?

Edit: This is actually a terrible idea, I think. If the main benefit is simplifying the pre-commit configuration, we can do that trivially by (a) requiring PDM for Openverse development, and (b) using a monorepo root-level pyproject.toml to install and configure pre-commit:

[tool.pdm.dev-dependencies]
dev = [
    "pre-commit",
]

And add pdm run pre-commit install to the install just recipe.

No need to download the .pyz, no need to worry about system Python at all. PDM is easy to install and it manages Python distributions too, so we can use whatever Python version we want.

As far as this particular issue with prettier goes, we'd just take pnpm to be a requirement for Openverse development, and run the hook locally. Or we can run it in Docker if for some reason we really want to avoid pnpm assumed on the host.

sarayourfriend commented 3 weeks ago

I've unassigned myself from this because I haven't had time to directly work on it since the sprint to get ov working.

I still like the idea to start running prettier directly in the execution environment. We'll want to make sure it's always available in ov, but because we already rely on pnpm install to have ESLint available to run in CI linting, we'll definitely have prettier available if we add it to the root package.json.

@dhruvkb any opinions here?

dhruvkb commented 3 weeks ago

I agree with your most recent comment and think that going the way of ESLint would be ideal. It'll be an extension of a practice that we've already determined to work well.

We can add Prettier and its plugins as dev dependencies in the root package.json and then invoke it as a language: system hook, similar to what we do for ESLint:

https://github.com/WordPress/openverse/blob/6dbbe5e1460926157042d5b58554f61a876ad488/.pre-commit-config.yaml#L126-L132

As an added bonus, we also won't have to manually sync the package versions between the pnpm-lock.yaml and the .pre-commit-config.yaml files.

sarayourfriend commented 3 weeks ago

Perfect, exactly what I had in mind :rocket:

sarayourfriend commented 3 weeks ago

I was going to update the PR description to specify how to do this, but it was just as much work as actually just doing it. So PR is here now for this: https://github.com/WordPress/openverse/pull/4749