Skip to content

Remove unnecessary files by adding .npmignore#37

Open
Mehulantony wants to merge 4 commits intomainfrom
Issue-35
Open

Remove unnecessary files by adding .npmignore#37
Mehulantony wants to merge 4 commits intomainfrom
Issue-35

Conversation

@Mehulantony
Copy link
Collaborator

Added .npmignore file to exclude non-production assets (docs, tests, backups, and local configuration files).

@Mehulantony
Copy link
Collaborator Author

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.

Copy link
Collaborator

@thehabes thehabes left a comment

Choose a reason for hiding this comment

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

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
CODEOWNERS

Suggested Fix:

# Documentation
docs/
*.md
!README.md
CODEOWNERS

This 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.backup

Suggested Fix:

*.backup
*.bak
*.swp
*.tgz

How 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants