Skip to content

chore: add opt-in pre-commit lint --write hook#2475

Merged
zimeg merged 10 commits intoslackapi:mainfrom
zi0w:chore/precommit-lint-write
Mar 11, 2026
Merged

chore: add opt-in pre-commit lint --write hook#2475
zimeg merged 10 commits intoslackapi:mainfrom
zi0w:chore/precommit-lint-write

Conversation

@zi0w
Copy link
Contributor

@zi0w zi0w commented Jan 25, 2026

Summary

Fixes #2034

Adds an opt-in Git pre-commit hook that runs lint auto-fix (lint:fix or lint -- --fix) for only the packages touched in staged changes.
Also documents how to enable it via core.hooksPath in the maintainer’s guide.

Requirements (place an x in each [ ])

Changes

  • Add .githooks/pre-commit (opt-in): runs package-level lint auto-fix for staged changes only
  • Update .github/maintainers_guide.md: document how to enable the hook

Test plan

  1. Enable hook: git config core.hooksPath .githooks
  2. Modify a file under packages//... and stage it
  3. Run git commit and confirm the hook runs lint auto-fix and the commit succeeds.
test-image

@zi0w zi0w requested a review from a team as a code owner January 25, 2026 13:07
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @zi0w to sign the Salesforce Inc. Contributor License Agreement.

@zi0w
Copy link
Contributor Author

zi0w commented Jan 25, 2026

I’ve signed the Salesforce CLA.
When you have a moment, could a maintainer approve the workflow run for this PR?

@codecov
Copy link

codecov bot commented Jan 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.67%. Comparing base (75649f4) to head (eda1fdc).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2475   +/-   ##
=======================================
  Coverage   90.67%   90.67%           
=======================================
  Files          82       82           
  Lines       14994    14994           
  Branches      508      508           
=======================================
  Hits        13596    13596           
  Misses       1373     1373           
  Partials       25       25           
Flag Coverage Δ
cli-hooks 90.67% <ø> (ø)
cli-test 90.67% <ø> (ø)
logger 90.67% <ø> (ø)
oauth 90.67% <ø> (ø)
socket-mode 90.67% <ø> (ø)
web-api 90.67% <ø> (ø)
webhook 90.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hello-ashleyintech hello-ashleyintech self-requested a review January 26, 2026 18:42
@hello-ashleyintech
Copy link
Contributor

hi, @zi0w! Thank you so much for the contribution. I have run the workflow run for this PR, and it looks like there are a few failing tests across Node versions 18, 20, and 22 for the logger, web-api, and cli-test packages. Could you take a look at these failures at your earliest convenience?

Also, please confirm that you have signed the CLA using the same email and GitHub account as the one you've submitted the PR for - it looks like that check failed as well, so just want to verify whether I need to close or re-open the PR if this is reporting a false result.

@@ -0,0 +1,54 @@
#!/bin/sh
set -e
Copy link
Contributor

Choose a reason for hiding this comment

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

this implementation is very complex - is there a way we could simplify this?

@zi0w
Copy link
Contributor Author

zi0w commented Jan 27, 2026

Hi @hello-ashleyintech — thanks for running the workflows!

CLA: I already signed the Salesforce CLA using the same email/account as this PR, but it still shows cla:missing. Do you know if there’s a way for me to retry or trigger a re-check, or anything else I should do to resolve it?

CI failures: I’m looking into the Windows failures (Node 18/20/22) for logger, web-api, and cli-test now. I’ll review the logs and push an update once I find the cause.

Hook complexity: Agreed it’s a bit heavy. I’ll simplify the hook logic while keeping the same behavior (only run for staged packages/ changes + auto-fix + re-stage) and include that in the next commit.

@zi0w
Copy link
Contributor Author

zi0w commented Jan 27, 2026

Hi, @hello-ashleyintech!

Quick update:

CLA: Looks like the CLA check is now passing after the latest commit — thank you!

Hook complexity: I simplified the pre-commit hook to reduce parsing/branching while keeping the behavior the same. It derives the touched packages/ from staged files, runs lint:fix when available (otherwise falls back to lint -- --fix), and re-stages any auto-fixes. I also adjusted it so the lint output is visible during commits to make local verification/debugging easier.

CI failures: The Windows jobs were failing very quickly (~a few seconds) with the same message:
##[error]Could not find a part of the path 'D:\a'.
Given how quickly it fails and that the message points to the workspace path, this might be related to the GitHub Actions runner/workflow environment rather than the changes in this PR — but I don’t want to assume. Would you mind re-running the workflow once to see if it reproduces? If it still fails, I’ll revisit it based on the rerun result and we can figure out the next best step.

Thanks again for taking a look!

@hello-ashleyintech
Copy link
Contributor

@zi0w I have closed and re-opened the PR to bump the CLA check, and it looks like it's now working. I will now approve the workflows to run!

@zimeg
Copy link
Member

zimeg commented Jan 27, 2026

👋 #2478 might make adjacent changes related to linting in a monorepo. FWIW I'm not familiar with pre lint hooks-

@zi0w
Copy link
Contributor Author

zi0w commented Jan 28, 2026

Hi @zimeg — thanks for the heads-up!

Yep, this PR is an opt-in pre-commit hook that runs lint --write (via each package’s lint:fix / lint -- --fix) only for staged changes, and re-stages any auto-fixes. It shouldn’t change CI behavior directly, but I agree it’s adjacent to monorepo linting/workspace changes like #2478.

If there’s anything you’d like me to align with (e.g., preferred package detection or how lint commands should be invoked in the monorepo), I’m happy to adjust.

@zi0w
Copy link
Contributor Author

zi0w commented Jan 28, 2026

Hi @hello-ashleyintech — CLA check is now passing and all CI checks are green.

I also simplified the opt-in pre-commit hook while keeping the same behavior (lint:fix → fallback to lint -- --fix, then re-stage).
When you have a moment, would you be able to take a look / review the latest changes?

Thanks again!

@zi0w
Copy link
Contributor Author

zi0w commented Feb 6, 2026

Hi @hello-ashleyintech — just a gentle ping on this when you have a moment.

Since my last update, CLA is passing and all checks are green. Would you be able to take a quick look / leave a review when you get a chance? Thanks so much!

@hello-ashleyintech
Copy link
Contributor

hi @zi0w, apologies for the delay! It looks like this branch has a merge conflict, could you resolve it?

I discussed this solution with the team and we had a couple feedback items:

  • could we simplify this to simply be a minimal hook that runs npm run lint before the commit happens?
  • could we also revise this to lint at the root of the directory rather than within packages?

let me know if you have any questions!

@zi0w zi0w force-pushed the chore/precommit-lint-write branch from c0d8de3 to b3cea66 Compare February 27, 2026 14:38
@changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2026

⚠️ No Changeset found

Latest commit: eda1fdc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@zi0w
Copy link
Contributor Author

zi0w commented Feb 27, 2026

Hi @hello-ashleyintech — quick note: I’m still looking into this, but I’m about to log off for today. I’ll follow up with a quick update tomorrow or the day after. Thanks!

@zi0w
Copy link
Contributor Author

zi0w commented Mar 1, 2026

Hi @hello-ashleyintech! I updated the opt-in pre-commit hook to a minimal version that runs npm run lint from the repo root before commits, and revised the maintainer guide accordingly.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@zi0w Ahha since learning about this I've found I want this check more and more... CI keeps erroring for me! 😉

This is in a solid place but I'm leaving a few fast suggestions. Thanks so much for keeping this branch current as we're making changes alongside!

#!/bin/sh
set -e
cd "$(git rev-parse --show-toplevel)" || exit 1
npm run lint No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

👾 quibble: Can we end this file with a newline? It might not be linted outside of packages I believe...

@@ -0,0 +1,4 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#!/bin/sh
#!/usr/bin/env bash

🌲 suggestion: This might be a more standard path to use across setups!

Comment on lines +21 to +39
#### Pre-commit lint hook (optional)
We provide an opt-in Git hook that runs `npm run lint` from the repository root before a commit is created.

Enable it once per clone:

```sh
git config core.hooksPath .githooks
```

Disable it:

```sh
git config --unset core.hooksPath
```

Notes:
- The hook runs `npm run lint` from the repository root.
- You can skip it with `git commit --no-verify` if needed.

Copy link
Member

Choose a reason for hiding this comment

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

📚 suggestion: Let's add this as a new task before the "testing and linting" section! Perhaps as:

📠 Project Setup

Clone this repository with the following command:

git clone https://github.com/slackapi/node-slack-sdk

With more details on hooks in following sentences?

Copy link
Member

Choose a reason for hiding this comment

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

👁️‍🗨️ question: Would moving this to the paths following be better for organization or is ".githooks" standard for discovering these scripts?

scripts/hooks/...

@zi0w
Copy link
Contributor Author

zi0w commented Mar 4, 2026

Hi, @zimeg! Thanks for the quick suggestions!

  • Updated the hook file to end with a newline.
  • Switched the shebang to #!/usr/bin/env bash as suggested.
  • Moved the pre-commit hook instructions into a dedicated optional “Git hooks” task section (before the testing/linting section) to make it easier to discover.

I kept the directory as .githooks since it’s explicitly enabled via core.hooksPath and seems to be a common convention, but happy to move it to something like scripts/hooks if you’d prefer.

@zimeg zimeg added tests M-T: Testing work only semver:patch labels Mar 11, 2026
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@zi0w LGTM! Thanks a ton for the changes and sharing standards toward git hooks! I learned a few new things 🎁

Let's appreciate fewer CI fails across matrix with this!

@zimeg zimeg merged commit a2eb4be into slackapi:main Mar 11, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

monorepo: add a linter --write precommit hook

3 participants