Improve the move

[Judahmeek]

This pull request makes several significant updates to the react-on-rails-pro-node-renderer package and its workspace configuration, focusing on improving package exports, dependency management, build/test scripts, and code quality tooling. The changes also remove obsolete files and update references to streamline the development and build process.

Package structure and configuration:

• Major overhaul of packages/react-on-rails-pro-node-renderer/package.json: removes deprecated fields, adds an exports map for proper subpath exports (including integrations), sets up resolutions for problematic dependencies, updates scripts to use nps and Yarn consistently, and introduces Jest and Husky configuration directly in the package. [1] [2]
• Removes the placeholder react_on_rails_pro/package.json and the package-specific tsconfig.json, consolidating configuration and eliminating redundancy. [1] [2]

Dependency and workspace management:

• Updates dependency handling in knip.ts to better reflect actual usage: removes unnecessary test and runtime dependencies, refines entry points (including new integrations glob), and improves ignore/include logic for dependencies and files. Also enables ignoreExportsUsedInFile for more accurate unused export checks. [1] [2] [3] [4] [5]
• Updates .prettierignore and workspace ignore files to reflect new test fixture locations and remove unnecessary exclusions. [1] [2]

Build and test scripts:

• Standardizes usage of yarn run in root and workflow scripts for consistency and reliability in all environments. [1] [2]
• Updates the package-scripts.yml build check to support multiple output files, improving robustness for different build targets.

Code and import improvements:

• Removes usage of import ... with { type: 'json' } for JSON imports, reverting to standard imports for better compatibility. [1] [2]
• Updates test imports to use relative paths, improving portability and correctness. [1] [2]
• Exposes additional types (Notifier, ErrorNotifier, MessageNotifier) from errorReporter.js for integrations.
• Adds an @internal tag to the resetVM export for clarity in test usage.

Dependencies and peer dependencies:

• Updates and clarifies dependencies and peerDependencies in package.json files, including the addition of @jest/globals and development dependencies for testing and linting. [1] [2]

These changes collectively modernize the package's structure, improve maintainability, and ensure more reliable builds and tests across the monorepo.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch judahmeek/move-node-renderer-package-json

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

PR Review: Improve the move

I've reviewed this PR which modernizes the react-on-rails-pro-node-renderer package structure. Overall it's a solid refactor with excellent code quality improvements, but there are several critical issues that must be addressed before merging.

Critical Issues (MUST FIX)

1. Build Script Testing Required - Per CLAUDE.md requirements in .claude/docs/testing-build-scripts.md, you MUST manually test: (a) yarn run prepack, (b) yarn run yalc.publish, (c) verify build artifacts exist, (d) test clean install. This is non-negotiable as yalc publish failures have gone undetected for weeks before.

2. Jest Coverage Misconfiguration - collectCoverageFrom points to ./tests/ instead of ./src/. This is backwards - you want coverage from source code, not test files.

Important Issues (SHOULD FIX)

3. Missing files field - Removed from package.json which could cause bloated published packages including tests and source files.

4. Missing main and types fields - Removed in favor of only using exports, which breaks compatibility with older tools that don't fully support the exports field.

5. Build script logic - package-scripts.yml checks for ReactOnRails.full.js OR ReactOnRailsProNodeRenderer.js which could mask build failures in monorepo context.

Strengths

• Excellent: Modern exports map with proper subpath exports for integrations
• Excellent: Removal of deprecated import with type json syntax for better Node.js compatibility
• Excellent: Exposing Notifier types from errorReporter for integrations
• Excellent: Adding @internal JSDoc tag to resetVM
• Good: Consolidation of config into node-renderer package
• Good: Strategic use of resolutions to avoid unnecessary sentry-testkit dependencies
• Good: Standardizing on yarn run prefix

Action Items

Before merging:

1. MUST: Run manual build script tests per CLAUDE.md requirements
2. MUST: Fix Jest collectCoverageFrom to target src/ not tests/
3. SHOULD: Re-add files field to package.json
4. SHOULD: Re-add main and types fields for backwards compatibility
5. SHOULD: Verify TypeScript project references still work without composite: true
6. SHOULD: Verify sentry-testkit tests pass with empty dependency overrides

The code quality changes are excellent. Please address the critical testing requirement and Jest configuration bug before merging.

[claude]

PR Review: Improve the move

This PR successfully consolidates the react-on-rails-pro-node-renderer package structure, removing redundancy and improving maintainability. Overall, this is a solid refactoring with good attention to detail.

---

Strengths

1. Excellent Git History Preservation: Smart approach using git mv to preserve history by moving the old package.json to the new location before making changes.

2. Proper Exports Configuration: The new exports field in package.json correctly supports subpath exports for integrations, which is a modern and proper way to expose package APIs.

3. Import Attribute Removal: Removing with type json syntax improves compatibility. This syntax is still experimental and not well-supported across all environments.

4. Dependency Cleanup: Removing unnecessary ESLint/Babel devDependencies that are already in the root workspace reduces redundancy and potential version conflicts.

5. Type Export Improvements: Adding Notifier, ErrorNotifier, MessageNotifier exports improves the public API for integrations.

6. Documentation with JSDoc Tags: Adding @internal tag to resetVM is excellent API hygiene.

---

Critical Issues to Address

1. Build Script Testing Required (CRITICAL per CLAUDE.md)

From CLAUDE.md and testing-build-scripts.md, you MUST test build scripts manually before merging to prevent silent failures like the 7-week yalc publish bug documented in the testing guide.

Required tests:

• cd packages/react-on-rails-pro-node-renderer
• yarn run prepack (must succeed)
• yarn run yalc.publish (must succeed - CRITICAL)
• Verify artifacts exist: ls -la lib/ReactOnRailsProNodeRenderer.js

2. TypeScript Configuration Concerns

Issue: The package-level tsconfig.json was deleted, and build scripts now reference src/tsconfig.json. I don't see src/tsconfig.json in the diff. Did this file already exist, or does it need to be created? If it doesn't exist, the build will fail.

Recommendation: Verify this file exists or update build scripts to use the root tsconfig with overrides.

3. Import Path Breaking Changes

The test imports changed to use relative paths with eslint-disable-next-line import/no-relative-packages. Using lib/ directory imports couples tests to build artifacts. Tests will fail if the package isn't built first. The eslint-disable comments suggest this violates project conventions.

Consider if these should use source imports instead.

4. Dependency Resolution Workarounds

package.json adds resolutions for sentry-testkit dependencies using empty-npm-package. Why are these needed? Consider adding a comment explaining this workaround or documenting it in the changelog.

---

Additional Observations

1. Standardizing yarn run: The changes consistently use yarn run which is good for consistency.

2. @jest/globals: Added to root devDependencies. Ensure this is actually needed at root level.

3. Knip Configuration: The knip.ts changes correctly reflect the new package structure.

4. Missing files Field: The old package.json had a files field specifying what to publish. The new one doesn't. Is this intentional? Consider explicitly listing lib//*.js, lib//*.d.ts.

---

Testing Checklist (Per CLAUDE.md)

Before merging:

• yarn run prepack succeeds
• yarn run yalc.publish succeeds
• yarn run build creates expected artifacts in lib/
• yarn run test passes
• yarn run type-check passes
• Confirm src/tsconfig.json exists or build scripts are updated
• Test in a clean clone: rm -rf node_modules && yarn install
• Run RuboCop: bundle exec rubocop (MANDATORY)

---

Overall Assessment

This is quality refactoring work with good attention to monorepo structure and modern package practices. The main risks are:

1. Untested build scripts (per project docs, this has caused silent failures before)
2. Potential TypeScript config missing (build will fail)
3. Test import coupling to build artifacts (could cause CI issues)

Once the critical build testing is completed and the TypeScript config is verified, this should be safe to merge.

Recommendation: Request changes to verify build scripts work and TypeScript config exists.

[claude]

Code Review: PR #2078 - Improve the move

This PR makes significant improvements to the monorepo structure by consolidating the node-renderer package configuration. Overall, the changes are well-structured and improve maintainability. Here's my detailed feedback:

✅ Strengths

1. Package Structure Improvements

   • ✅ Good addition of proper exports field with subpath exports for ./integrations/*
   • ✅ Moving configuration closer to the source (src/tsconfig.json) is a solid pattern
   • ✅ Removing redundant react_on_rails_pro/package.json placeholder eliminates confusion
   • ✅ Consolidating Jest and Husky config into the package.json simplifies setup

2. Dependency Management

   • ✅ Using resolutions to exclude unused transitive deps (express/body-parser from sentry-testkit) is smart
   • ✅ Removing react and react-dom from peerDependencies is correct - this is a server-side package
   • ✅ Moving pino-pretty to devDependencies is appropriate (dev/debug only)
   • ✅ Exposing additional types (Notifier, ErrorNotifier, MessageNotifier) for integrations is good API design

3. CI/Workflow Improvements

   • ✅ Standardizing on yarn run prefix throughout is consistent and explicit
   • ✅ New ci script in package.json with proper Jest reporters
   • ✅ Using workspace-specific command (yarn workspace react-on-rails-pro-node-renderer run ci) is more maintainable
   • ✅ Removing unnecessary webpack cache in pro-test workflow simplifies the pipeline

4. Code Quality

   • ✅ Removing import ... with { type: 'json' } syntax improves compatibility (this syntax is Stage 3, not fully stable)
   • ✅ Using relative imports in tests (../../react-on-rails/lib/types) is more portable
   • ✅ Adding @internal JSDoc tag to resetVM clarifies test-only usage
   • ✅ Knip configuration cleanup removes unnecessary test dependencies from root

⚠️ Concerns & Recommendations

1. CRITICAL: Missing Build Verification

Per CLAUDE.md "Testing Build Scripts" guidelines:

# You MUST test these before merging:
yarn run prepack
yarn run yalc.publish

# Verify artifacts exist at expected paths:
ls -la packages/react-on-rails-pro-node-renderer/lib/ReactOnRailsProNodeRenderer.js

The package-scripts.yml change adds lib/ReactOnRailsProNodeRenderer.js as an expected build artifact. Have you verified this file is created at this path after build? If not, the prepack script will silently fail.

2. TypeScript Configuration Change

• Removed: Root-level tsconfig.json (extended from ../../tsconfig.json)
• Added: src/tsconfig.json (extends @tsconfig/node14)

Question: Does this change affect type-checking behavior? The new config uses module: "nodenext" which may have different resolution rules. Have you verified:

• yarn run type-check passes?
• Build output types are correct?
• No breaking changes for consumers?

3. Dependency Version Concerns

"devDependencies": {
  "react-on-rails": "./packages/react-on-rails",
  // ...
}

Using a relative path for react-on-rails is fine for local development but could cause issues:

• Publishing: Will this work when published to npm?
• External consumers: Users installing from npm won't have this path available

Recommendation: Consider using workspace:* protocol or ensuring this is properly handled during publishing.

4. Test Path Changes

The test imports changed from package references to relative paths:

// Old:
import { RSCPayloadChunk } from 'react-on-rails';

// New:  
import { RSCPayloadChunk } from '../../react-on-rails/lib/types';

Concern: This couples tests to build output location. If the build hasn't run, tests will fail with cryptic module not found errors. Consider:

• Adding a pre-test build step
• Or using a Jest moduleNameMapper to resolve package names

5. peerDependencies Removal

Removed react and react-dom from peerDependencies, which makes sense for a Node.js renderer. However:

• Question: Are these dependencies still needed at runtime for server-side rendering?
• If yes: They should remain (even if optional)
• If no: ✅ Removal is correct

6. Knip Configuration

Removed many test/runtime dependencies from root ignoreDependencies:

-'@testing-library/dom',
-'@testing-library/jest-dom',
-'create-react-class',
-'redux',

Question: Have you run yarn run knip to ensure no new unused dependency warnings appear? These may have been legitimately used transitively.

7. Documentation Gap

The PR description mentions removing "obsolete files" but doesn't specify:

• Why was react_on_rails_pro/package.json obsolete?
• What was its original purpose?
• Why is it safe to remove now?

Recommendation: Add a brief explanation in the PR description or a comment in the commit history for future reference.

🧪 Testing Recommendations

Before merging, please verify:

1. Build and Package Scripts:

   cd packages/react-on-rails-pro-node-renderer
   yarn run clean
   yarn run build
   ls -la lib/ReactOnRailsProNodeRenderer.js  # Should exist
   yarn run prepack  # Should succeed
   yarn run yalc.publish  # Should succeed

2. Type Checking:

   yarn run type-check  # Should pass with zero errors

3. Tests:

   yarn run test  # Should pass
   yarn run ci    # Should pass with junit output

4. Knip:

   yarn run knip  # Check for new unused dependency warnings

5. CI Simulation:

   # Per CLAUDE.md, replicate CI failures locally
   bin/ci-rerun-failures  # If any CI failures exist

📝 Minor Issues

1. Line 78: "developing": "nps node-renderer.debug" - Is this script defined in package-scripts.yml? I don't see it in the diff.

2. Prettier ignore: Added packages/react-on-rails-pro-node-renderer/tests/fixtures/projects but is this directory created/populated anywhere?

3. License field: "license": "UNLICENSED" - Is this intentional for Pro package? Confirm this aligns with licensing strategy.

🎯 Summary

This is a solid refactoring that improves the monorepo structure. The main risks are:

1. Build script verification (CRITICAL per CLAUDE.md)
2. TypeScript config changes potentially affecting consumers
3. Relative path imports in tests coupling to build output

Recommendation: Once the build scripts are verified and tests pass, this should be safe to merge. Consider adding a CI job that validates yalc publish succeeds to prevent future regressions.

---

Overall Assessment: 👍 Approve pending verification of build scripts and test execution