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

fix(husky): use pnpx #1975

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(husky): use pnpx #1975

wants to merge 1 commit into from

Conversation

xav-ie
Copy link

@xav-ie xav-ie commented Mar 9, 2025

Defaulting to npx causes users to unexpectedly also have npx installed. It caused my commit staging to break.

PR Checklist

Overview

Simply replaces the npx lint-staged command to use pnpx lint-staged.

Please, let me know if I need to scale back my changes. I am trying to keep this PR minimal and just get the project working. It was not yet working for me with this dev environment:

{ ... }: {
  languages.javascript = {
    enable = true;
    # I should not have to enable `npm`, we use `pnpm` in this house!
    # npm.enable = true;
    pnpm.enable = true;
  };
}

I have a follow-up PR here:
#1976
It goes "full-pnpx" because I don't think we should expect users to have both npx and pnpx installed. pnpx in my experience is also way faster since it copies from global store, just like Nix :)))))

It also seems similar in spirit to this PR:
#1800

I am not sure why the "full-pnpx" move was not taken. Maybe I can learn why not, or just oversight.

🐢

@xav-ie xav-ie marked this pull request as ready for review March 9, 2025 02:29
Defaulting to npx causes users to unexpectedly also have npx installed.
@xav-ie xav-ie mentioned this pull request Mar 9, 2025
3 tasks
@@ -93,7 +93,7 @@ Here we'll outline the steps required to migrate a CTA app to a GitHub Action:
```diff
+pnpm run build
+git add dist
npx lint-staged
pnpx lint-staged
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be reverted, but I kept this change because pnpx is just better. It's faster and what the project will be using after this commit.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faster

Is it, though? Trying out an arbitrary text addition of \nExample.\n to the bottom of README.md:

joshgoldberg ~/repos/create-typescript-app $ hyperfine "npx lint-staged" --warmup 3
Benchmark 1: npx lint-staged
  Time (mean ± σ):     843.7 ms ±   9.0 ms    [User: 833.9 ms, System: 206.5 ms]
  Range (min … max):   832.7 ms … 864.0 ms    10 runs
 
joshgoldberg ~/repos/create-typescript-app $ hyperfine "pnpx lint-staged" --warmup 3
Benchmark 1: pnpx lint-staged
  Time (mean ± σ):     974.1 ms ±  22.9 ms    [User: 854.3 ms, System: 185.1 ms]
  Range (min … max):   943.5 ms … 1007.2 ms    10 runs

System info:

  • OS: macOS Ventura 13.2
  • Hardware: Mac Studio, Apple M1 Max, 32 GB
  • Node: 22.12.0
  • npm: 10.9.0
  • pnpm: 10.5.2

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this @xav-ie! This is the first time I've heard of a system having pnpx but not having npx. But I also think you're the first (known) power Nix user I've gotten an issue or PR from 😄.

Request: could you please file an issue with a reproduction I can play with? I'm not a Nix user myself and don't even know how to make a system with Node and not npm.

If pnpx really is a little slower than npx, my hunch is we'll probably want to turn this into an option that defaults to whichever one is installed on the user's computer. I'm under the impression npx is generally meant to be assumed as existing for general Node systems? But maybe that's incorrect and we should reduce the number of dependencies to just have pnpx?

@@ -93,7 +93,7 @@ Here we'll outline the steps required to migrate a CTA app to a GitHub Action:
```diff
+pnpm run build
+git add dist
npx lint-staged
pnpx lint-staged
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faster

Is it, though? Trying out an arbitrary text addition of \nExample.\n to the bottom of README.md:

joshgoldberg ~/repos/create-typescript-app $ hyperfine "npx lint-staged" --warmup 3
Benchmark 1: npx lint-staged
  Time (mean ± σ):     843.7 ms ±   9.0 ms    [User: 833.9 ms, System: 206.5 ms]
  Range (min … max):   832.7 ms … 864.0 ms    10 runs
 
joshgoldberg ~/repos/create-typescript-app $ hyperfine "pnpx lint-staged" --warmup 3
Benchmark 1: pnpx lint-staged
  Time (mean ± σ):     974.1 ms ±  22.9 ms    [User: 854.3 ms, System: 185.1 ms]
  Range (min … max):   943.5 ms … 1007.2 ms    10 runs

System info:

  • OS: macOS Ventura 13.2
  • Hardware: Mac Studio, Apple M1 Max, 32 GB
  • Node: 22.12.0
  • npm: 10.9.0
  • pnpm: 10.5.2

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants