Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sarayourfriend opened this issue May 2, 2024 · 6 comments · Fixed by #4749
Closed

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

sarayourfriend opened this issue May 2, 2024 · 6 comments · Fixed by #4749
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: mgmt Related to repo management and automations

Comments

@sarayourfriend
Copy link
Collaborator

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

@sarayourfriend sarayourfriend added 🟨 priority: medium Not blocking but should be addressed soon 🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🧱 stack: mgmt Related to repo management and automations labels May 2, 2024
@openverse-bot openverse-bot moved this to 📋 Backlog in Openverse Backlog May 2, 2024
@dhruvkb
Copy link
Member

dhruvkb commented May 2, 2024

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
Copy link
Collaborator Author

sarayourfriend commented May 9, 2024

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 sarayourfriend self-assigned this May 21, 2024
@openverse-bot openverse-bot moved this from 📋 Backlog to 📅 To Do in Openverse Backlog May 21, 2024
@openverse-bot openverse-bot moved this from 📅 To Do to 🏗 In Progress in Openverse Backlog May 21, 2024
@sarayourfriend sarayourfriend removed their assignment Aug 12, 2024
@sarayourfriend
Copy link
Collaborator Author

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
Copy link
Member

dhruvkb commented Aug 12, 2024

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:

- id: eslint
name: eslint
files: (frontend|automations|packages/js).*?\.(js|ts|mjs|vue|json5|json)$
"types": [file] # ESLint only accepts [javascript] by default.
language: system
pass_filenames: false
entry: bash -c 'pnpm run eslint --fix'

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
Copy link
Collaborator Author

Perfect, exactly what I had in mind 🚀

@sarayourfriend
Copy link
Collaborator Author

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: #4749

@openverse-bot openverse-bot moved this from 🏗 In Progress to ✅ Done in Openverse Backlog Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: mgmt Related to repo management and automations
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants