Skip to content

Conversation

@alexeyr-ci2
Copy link
Collaborator

@alexeyr-ci2 alexeyr-ci2 commented Oct 27, 2025

Summary

The .npmignore files don't do anything in practice since all our packages have files in package.json, the .npmignore files are at the root, and https://docs.npmjs.com/cli/v11/configuring-npm/package-json#files says

You can also provide a .npmignore file in the root of your package or in subdirectories, which will keep files from being included. At the root of your package it will not override the "files" field, but in subdirectories it will.

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

This change is Reviewable

Summary by CodeRabbit

  • Chores
    • Modified npm package exclusion patterns affecting which files are included in published distributions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

Walkthrough

Two .npmignore configuration files were modified to remove ignore patterns, causing previously excluded files and directories to be included in npm package distributions across multiple project areas.

Changes

Cohort / File(s) Summary
NPM ignore pattern removal
.npmignore, react_on_rails_pro/.npmignore
Removed multiple ignore entries including documentation (CHANGELOG.md, README.md), configuration files (Gemfile, Rakefile, docker-compose.yml), source directories (app, lib, bin, rakelib), test/build artifacts (coverage, spec, tmp), and development tools (Dockerfile_tests, ruby-lint.yml). The react_on_rails_pro/.npmignore also removed Node.js renderer packages paths (packages/node-renderer/src, packages/node-renderer/tests).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Two homogeneous configuration files with identical change patterns
  • No logic modifications, only file inclusion list adjustments
  • Verify intent: confirm that exposing documentation, build artifacts, and source files in npm packages is intentional and won't inflate package size unnecessarily

Poem

🐰 Files that hid in the shadows deep,
Now bundled close, no longer to keep,
npm packages grow round and complete,
With docs and examples, bittersweet! 📦✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Remove .npmignore files" directly references the main subject of the changeset and clearly indicates the primary action being taken. The changes shown in the raw summary confirm that patterns from both the root .npmignore and react_on_rails_pro/.npmignore files were removed, and the PR objectives explicitly state that the goal is to remove these .npmignore files because they are ineffective in practice. The title is concise, specific enough for a developer reviewing the repository history to understand the primary change, and avoids vague or generic language.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch alexeyr/remove-npmignore

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d87ea7 and b7d0842.

📒 Files selected for processing (2)
  • .npmignore (0 hunks)
  • react_on_rails_pro/.npmignore (0 hunks)
💤 Files with no reviewable changes (2)
  • .npmignore
  • react_on_rails_pro/.npmignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: dummy-app-integration-tests (3.2, 20)
  • GitHub Check: dummy-app-integration-tests (3.4, 22)
  • GitHub Check: build-dummy-app-webpack-test-bundles
  • GitHub Check: rspec-package-tests (3.4, latest)
  • GitHub Check: rspec-package-tests (3.2, minimum)
  • GitHub Check: rspec-package-tests (3.2, latest)
  • GitHub Check: rspec-package-tests (3.4, minimum)
  • GitHub Check: lint-js-and-ruby
  • GitHub Check: build-dummy-app-webpack-test-bundles

Comment @coderabbitai help to get the list of available commands and usage tips.

@alexeyr-ci2 alexeyr-ci2 changed the title Remove .npmignore files Remove .npmignore files Oct 27, 2025
@alexeyr-ci2 alexeyr-ci2 marked this pull request as ready for review October 27, 2025 17:19
@claude
Copy link

claude bot commented Oct 27, 2025

Code Review: Remove .npmignore files

Summary

This PR removes .npmignore files from the root and react_on_rails_pro/ directories. The author's rationale is correct - these files are redundant because both packages already define explicit files fields in their package.json files.

Analysis

Correctness of the Change

The change is technically correct. According to the npm documentation:

You can also provide a .npmignore file in the root of your package or in subdirectories, which will keep files from being included. At the root of your package it will not override the "files" field, but in subdirectories it will.

Since both packages have explicit files fields defined in their package.json files (packages/react-on-rails/package.json:67-72 and packages/react-on-rails-pro/package.json:69-72), the .npmignore files at the root level are indeed ineffective and can be safely removed.

Quality and Best Practices

  • Reduces maintenance burden: Fewer files to maintain
  • Single source of truth: The files field in package.json is clearer and more explicit
  • Industry standard: Using the files allowlist approach is considered best practice over blocklist approaches like .npmignore

Testing and Validation

The project has automated package validation via publint in CI (.github/workflows/lint-js-and-ruby.yml:90). This will verify the package contents are correct after the change. The CI will catch any issues if the .npmignore removal unexpectedly affects package contents.

Minor Observation

The Pro package lint workflow (.github/workflows/pro-lint.yml) does not include publint checks. While this PR is safe, it would be beneficial to add similar validation for the Pro package to ensure consistency.

No Security Concerns

No security issues identified. The explicit files allowlist approach is actually more secure than blocklist approaches since it is harder to accidentally include sensitive files.

Recommendations

  1. Approve and merge - This is a good cleanup change
  2. Consider adding publint validation for the Pro package in a follow-up PR
  3. No need for CHANGELOG entry - This is an internal/infrastructure change with no user-facing impact

Checklist Review

The author correctly marked the checklist items as not applicable:

  • No tests needed (infrastructure change, validated by existing CI)
  • No docs needed (internal change)
  • No CHANGELOG needed (no user-facing impact)

Overall: LGTM

This is a safe, beneficial cleanup that removes redundant configuration files.

@alexeyr-ci2 alexeyr-ci2 merged commit 2398430 into master Oct 27, 2025
23 of 24 checks passed
@alexeyr-ci2 alexeyr-ci2 deleted the alexeyr/remove-npmignore branch October 27, 2025 17:45
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.

4 participants