Remove unnecessary files by adding .npmignore#37
Conversation
|
Ran "npm pack" to preview the published tarball contents and identified unnecessary files being included, such as tests, mocks, tooling directories, etc. Refined the .npmignore file to ensure that the tarball now includes only required runtime assets, while verifying that development-only files such as tests/, mocks/, and backup files are excluded. |
There was a problem hiding this comment.
npm run runtest does not pass,
Test Suites: 4 failed, 1 skipped, 10 passed, 14 of 15 total
Tests: 3 failed, 4 skipped, 10 passed, 17 total
Static Review Comments
Branch: Issue-35
Review Date: 2026-02-24
Reviewer: Claude Code assisted by Bryan - @thehabes
Bryan and Claude make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.
Summary
This PR adds a new .npmignore file (34 lines) to exclude non-production assets from the npm tarball. The intent is good — the pack output is now down to the essentials (~53 files, 537 KB). However, the file contains several logic errors that cause rules to silently malfunction, duplicate entries, and a negation pattern that doesn't actually negate anything.
Verified via npm pack --dry-run: the tarball currently includes only runtime files, README.md, and LICENSE. So the outcome is largely correct despite the bugs, but the file should still be fixed so its rules match the author's intent and don't cause surprises later.
| Category | Issues Found |
|---|---|
| 🔴 Critical | 0 |
| 🟠 Major | 2 |
| 🟡 Minor | 2 |
| 🔵 Suggestions | 1 |
Major Issues 🟠
Issue 1: Negation of !README.md is immediately overridden by explicit ignore rules
File: .npmignore:3-7
Category: Logic Error
Problem:
Lines 3–7 read:
*.md
!README.md
CONTRIBUTING.md
CODE_OF_CONDUCT.md
CODEOWNERS
The intent is "ignore all .md files except README.md." The !README.md negation on line 4 does work — npm's ignore parser processes top-to-bottom, so !README.md re-includes it after *.md excludes it.
However, lines 5–6 (CONTRIBUTING.md, CODE_OF_CONDUCT.md) are redundant dead rules. They are already excluded by the *.md glob on line 3. Listing them again after the negation doesn't hurt, but it's misleading — it reads as if these files were somehow not caught by *.md and need to be explicitly listed. This creates confusion for future contributors who might think the *.md glob is insufficient.
Worse, CODEOWNERS (line 7) has no file extension and is therefore not matched by *.md. It looks like it was placed here to be excluded, but it is grouped under the "Documentation" comment with no clear rationale for its placement after the negation line.
Current Code:
# Documentation
docs/
*.md
!README.md
CONTRIBUTING.md
CODE_OF_CONDUCT.md
CODEOWNERSSuggested Fix:
# Documentation
docs/
*.md
!README.md
CODEOWNERSThis is cleaner: *.md handles all markdown files, !README.md re-includes the one you want, and CODEOWNERS is explicitly listed since it's not caught by a glob.
How to Verify:
Run npm pack --dry-run 2>&1 | grep -iE '(readme|contributing|code_of|codeowners)' — only README.md should appear.
Issue 2: Duplicate .env and .env.* entries
File: .npmignore:26-27 and .npmignore:33-34
Category: Dead Code / Redundancy
Problem:
.env and .env.* each appear twice in the file — once under "Local configuration files" (lines 26–27) and again at the bottom (lines 33–34). npm's ignore parser uses last-match-wins semantics, so duplicates don't break anything, but they signal the file was edited without reviewing what already existed. This makes maintenance harder and can mask errors in the future (e.g., if someone adds a negation between the two blocks, the later duplicate would override it silently).
Current Code:
# Local configuration files
.env
.env.*
config.local.js
*.local.js
.github/
.claude/
.env
.env.*Suggested Fix:
Remove the duplicate lines 33–34, keeping only one occurrence under "Local configuration files":
# Local configuration files
.env
.env.*
config.local.js
*.local.js
# Tooling and CI
.github/
.claude/How to Verify:
Run npm pack --dry-run 2>&1 | grep -i env — no .env files should appear.
Minor Issues 🟡
Issue 3: Bottom section has no comment heading
File: .npmignore:31-34
Category: Code Hygiene
Problem:
The rest of the file is neatly organized with section comments (# Documentation, # Test and mock files, etc.), but the last 4 lines (.github/, .claude/, and the duplicate .env entries) have no section comment. This breaks the pattern and makes the file feel like lines were appended hastily.
Suggested Fix:
Add a section comment:
# Tooling and CI
.github/
.claude/Issue 4: db-controller.js.backup is redundant with *.backup
File: .npmignore:22
Category: Dead Code
Problem:
Line 19 already has *.backup which matches all files ending in .backup. The explicit db-controller.js.backup on line 22 is redundant.
Current Code:
*.backup
*.bak
*.swp
*.tgz
db-controller.js.backupSuggested Fix:
*.backup
*.bak
*.swp
*.tgzHow to Verify:
npm pack --dry-run 2>&1 | grep backup — should return nothing with or without the explicit line.
Suggestions 🔵
Suggestion 1: Consider using "files" in package.json as an allowlist instead
An alternative to .npmignore (a denylist) is the "files" field in package.json (an allowlist). An allowlist is generally safer because new files are excluded by default unless explicitly included. For example:
"files": [
"app.js",
"rest.js",
"utils.js",
"db-controller.js",
"auth/",
"bin/",
"controllers/",
"database/",
"public/",
"routes/"
]This is more explicit and less error-prone than maintaining ignore patterns. README.md, LICENSE, and package.json are always included automatically.
Added .npmignore file to exclude non-production assets (docs, tests, backups, and local configuration files).