Run "link-source" script in development environment only at node renderer package

[AbanoubGhadban]

…erer package

don't run the "link-source" script on preinstall of the node renderer package when it's installed at a client project and not running in dev environment

it depends on checking if yarn.lock file exist at installing directory to check if it's in dev environment

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

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

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

---

This change is 

Summary by CodeRabbit

• Chores
  • Improved installation robustness: the preinstall step now skips linking when no lockfile is present, and only runs linking when a lockfile exists.
  • Linking errors will now surface during installation when the lockfile is present, instead of being silently ignored.

[coderabbitai]

Walkthrough

Changed the preinstall script in react_on_rails_pro/package.json to first check for yarn.lock; if absent the script exits 0, otherwise it runs the original linking commands (yarn run link-source && yalc add --link react-on-rails) so errors from those commands will propagate when the lockfile exists.

Changes

Cohort / File(s)	Summary
NPM script configuration react_on_rails_pro/package.json	preinstall script updated to: `test -f yarn.lock

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant NPM as npm (preinstall)
    participant Shell as Shell
    participant LinkSrc as link-source
    participant Yalc as yalc

    NPM->>Shell: run preinstall
    Shell->>Shell: test -f yarn.lock
    alt yarn.lock missing
        Shell-->>NPM: exit 0 (no-op)
    else yarn.lock present
        Shell->>LinkSrc: yarn run link-source
        LinkSrc-->>Shell: success/failure
        Shell->>Yalc: yalc add --link react-on-rails
        Yalc-->>Shell: success/failure
        Shell-->>NPM: propagate exit status
    end

Loading

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

• Single-line change in an npm script; low complexity.
• Check: ensure the conditional behaves correctly on target shells (POSIX test -f and exit).

Suggested reviewers

• alexeyr-ci

Poem

🐰 I sniff the lock, a tiny hop,
If absent, pause — I do not stop.
If present, I fetch and link with care,
Errors now surface, bold and fair.
A carrot nod to safer fare. 🥕

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 pull request title "Run "link-source" script in development environment only at node renderer package" directly aligns with the main change implemented in the PR. The changeset modifies the preinstall script in react_on_rails_pro/package.json to conditionally execute the linking steps only when yarn.lock exists (indicating a development environment), preventing execution when the package is installed as a dependency in a client project. The title clearly captures this primary objective and is specific enough to convey the intent without unnecessary noise or vagueness.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/run-link-source-only-in-development-environment

---

📜 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 24dd92e and babd477.

📒 Files selected for processing (1)

• react_on_rails_pro/package.json (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Prettier is the sole authority for formatting all non-Ruby files; never manually format them

Files:

• react_on_rails_pro/package.json

⏰ 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). (10)

• GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: claude-review
• GitHub Check: rspec-package-tests (3.2, minimum)
• GitHub Check: build
• GitHub Check: rspec-package-tests (3.4, latest)
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: lint-js-and-ruby
• GitHub Check: rspec-package-tests (3.4, minimum)
• GitHub Check: rspec-package-tests (3.2, latest)

🔇 Additional comments (1)

react_on_rails_pro/package.json (1)

106-106: Implementation correctly addresses previous feedback.

The preinstall script now properly guards the linking commands behind a yarn.lock check using exit 0 for silent no-op in non-dev environments, while allowing errors from the linking steps to propagate when yarn.lock exists. This resolves the prior concern about suppressing legitimate errors and matches the suggested approach.

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

react_on_rails_pro/package.json (1)

106-106: Add a comment documenting the conditional behavior.

This script's logic (checking yarn.lock to detect development vs. client installation) is not self-evident. A brief inline comment will help future maintainers understand why the check exists.

-    "preinstall": "(test -f yarn.lock && yarn run link-source && yalc add --link react-on-rails) || true",
+    "preinstall": "test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails", 

And add a comment in an adjacent line or in the repo documentation explaining: "Only link source in development (presence of yarn.lock indicates development environment, not a client installation in node_modules)."

📜 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 24dd92e.

📒 Files selected for processing (1)

• react_on_rails_pro/package.json (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}

📄 CodeRabbit inference engine (CLAUDE.md)

Prettier is the sole authority for formatting all non-Ruby files; never manually format them

Files:

• react_on_rails_pro/package.json

⏰ 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). (10)

• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: build
• GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
• GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
• GitHub Check: rspec-package-tests (3.4, latest)
• GitHub Check: rspec-package-tests (3.2, latest)
• GitHub Check: rspec-package-tests (3.2, minimum)
• GitHub Check: rspec-package-tests (3.4, minimum)
• GitHub Check: lint-js-and-ruby
• GitHub Check: build-dummy-app-webpack-test-bundles

[claude]

Code Review - PR #1884

Thank you for working on this fix! This PR addresses an important issue where the link-source script should only run in development environments, not when the package is installed in client projects. Below is my comprehensive review:

---

✅ Positive Aspects

1. Problem Identification: The PR correctly identifies that the preinstall script shouldn't run development-specific operations when the package is installed by end users.

2. Simple Solution: Using the presence of yarn.lock as a heuristic for detecting the development environment is pragmatic.

3. Graceful Failure: The || true ensures installation doesn't fail, which is important for end-user experience.

---

⚠️ Critical Issues

1. Unreliable Detection Method

Location: react_on_rails_pro/package.json:106

Issue: Using test -f yarn.lock is not a reliable way to detect development vs. production environments:

• False Positives: Many client projects using this package will have their own yarn.lock file in their root directory, which means link-source would still run in their environment (defeating the purpose of this PR).
• Working Directory: The test -f yarn.lock command checks for yarn.lock in the current working directory during installation, which is typically the package's directory being installed, not the monorepo root.
• CI/CD Environments: CI systems often have yarn.lock files and would trigger the development script unnecessarily.

Evidence: I verified that yarn.lock exists in react_on_rails_pro/ directory, so this condition will be true during normal package installation.

2. Better Detection Methods

Consider these more reliable alternatives:

Option A - Check for specific development indicators:

"preinstall": "(test -d ../packages/react-on-rails && yarn run link-source && yalc add --link react-on-rails) || true"

This checks if the monorepo structure exists (the ../packages/react-on-rails directory).

Option B - Use environment variable:

"preinstall": "([ \"\" = \"true\" ] && yarn run link-source && yalc add --link react-on-rails) || true"

Developers set REACT_ON_RAILS_DEV=true in their environment.

Option C - Check if parent directory contains the monorepo marker:

"preinstall": "(test -f ../../.yalc.sig || test -f ../../../package.json && grep -q '@shakacode-tools/react-on-rails-pro' ../../../package.json && yarn run link-source && yalc add --link react-on-rails) || true"

Option D - Use npm/yarn-specific environment variables:

"preinstall": "node -e \"if (process.env.npm_config_local_prefix?.includes('react_on_rails_pro')) { require('child_process').execSync('yarn run link-source && yalc add --link react-on-rails', {stdio: 'inherit'}) }\" || true"

---

🐛 Potential Bugs

1. Silent Failures: The || true means all errors are silently ignored. While this prevents installation failures, it makes debugging difficult when the script should run but fails.

2. Undefined Behavior: What happens if link-source partially succeeds then fails? The state could be inconsistent.

---

📋 Missing Requirements (from CLAUDE.md)

Based on the project's CLAUDE.md guidelines, the following are mandatory before merging:

❌ 1. Tests

• No tests added to verify the preinstall behavior
• No tests to ensure the script runs in dev but not in production
• Recommendation: Add integration tests that simulate both development and production installations

❌ 2. Documentation

• CONTRIBUTING.md should be updated to explain the new behavior
• The PR description template sections are not completed (Summary, Pull Request checklist)
• Recommendation: Update docs/installation.md to explain what happens during installation

❌ 3. CHANGELOG

• CHANGELOG.md entry is missing
• Recommendation: Add entry describing this fix under "Unreleased" or appropriate version

❌ 4. Linting (CRITICAL)

• Must run bundle exec rubocop before commit
• Must ensure files end with newline characters
• Action Required: Run rake lint to verify all linting passes

---

🔒 Security Considerations

✅ No major security concerns, but note:

• The script executes shell commands, which is inherent to npm lifecycle scripts
• The || true prevents malicious exit codes from blocking installation
• No user input is processed, reducing injection risk

---

⚡ Performance Considerations

✅ Positive impact: This change will improve installation performance for end-users by skipping unnecessary development operations (building packages with yarn and yalc publish).

---

🎯 Recommendations Summary

High Priority (Must Fix)

1. Replace the detection method - test -f yarn.lock is unreliable
2. Add tests - Verify behavior in both dev and production contexts
3. Complete documentation - Update CHANGELOG, CONTRIBUTING.md
4. Complete PR checklist - Fill out the PR template sections

Medium Priority (Should Fix)

5. Improve error handling - Consider logging when the script is skipped vs. when it fails
6. Add comments - Explain the detection logic in package.json or accompanying docs

Nice to Have

7. Consider consistency - Check if spec/dummy/package.json and spec/execjs-compatible-dummy/package.json need similar fixes

---

🧪 Testing Suggestions

# Test 1: Verify it runs in development
cd react_on_rails_pro
yarn install --force
# Should see yalc output

# Test 2: Simulate production install
# (This needs a better test harness)
cd /tmp
npm init -y
npm install /path/to/react_on_rails_pro
# Should NOT run link-source

# Test 3: Verify CI behavior
# Run in CI environment to ensure it doesn't break builds

---

📝 Action Items for PR Author

Before this PR can be merged, please:

1. ✅ Choose and implement a more reliable detection method
2. ✅ Add tests for the installation behavior
3. ✅ Update CHANGELOG.md with an entry for this fix
4. ✅ Update CONTRIBUTING.md to document the preinstall behavior
5. ✅ Complete the PR description template (Summary and checklist sections)
6. ✅ Run bundle exec rubocop and rake lint to ensure all checks pass
7. ✅ Ensure all files end with newlines

---

💡 Overall Assessment

Direction: ✅ Correct - This PR addresses a real issue
Implementation: ⚠️ Needs work - The detection method is flawed
Completeness: ❌ Incomplete - Missing tests, docs, and CHANGELOG

Recommendation: Request Changes - Please address the detection method and add required documentation/tests before merging.

---

Thank you for your contribution! Please let me know if you'd like help implementing any of these suggestions or if you have questions about the feedback.

[claude]

Code Review - PR #1884: Run "link-source" script in development environment only

Thank you for working on this fix! This PR addresses an important issue where the link-source script should only run in development environments, not when the package is installed in client projects.

---

✅ Positive Aspects

1. Problem Identification: The PR correctly identifies that the preinstall script shouldn't run development-specific operations when the package is installed by end users.
2. Simple Approach: Using a file presence check as a heuristic for detecting the development environment is pragmatic.
3. Non-Breaking: The change ensures installation doesn't fail for end users.

---

⚠️ Critical Issues

1. Unreliable Detection Method

Location: react_on_rails_pro/package.json:106

Issue: Using test -f yarn.lock is not reliable for detecting development vs. production:

• False Positives: Many client projects using this package will have their own yarn.lock file in their root directory. When npm/yarn installs a package, the working directory during the preinstall script execution is typically the parent project's directory, not the package being installed. This means the script would still run in client environments (defeating the purpose).
• Monorepo Structure: This repository has yarn.lock at the root (/home/runner/work/react_on_rails/react_on_rails/yarn.lock), which is correct, but when installed as a dependency, that file won't exist in client projects' node_modules/@shakacode-tools/react-on-rails-pro-node-renderer/ directory.

2. Better Detection Methods

Option A - Check for monorepo structure (RECOMMENDED):

"preinstall": "(test -d ../packages/react-on-rails && yarn run link-source && yalc add --link react-on-rails) || true"

This checks if the monorepo's packages/react-on-rails directory exists, which only happens in the development environment.

Option B - Check for multiple markers:

"preinstall": "(test -f ../../../CLAUDE.md && test -f ../../react_on_rails.gemspec && yarn run link-source && yalc add --link react-on-rails) || true"

Checks for repository-specific files that only exist in the monorepo root.

Option C - Environment variable:

"preinstall": "([ \"$REACT_ON_RAILS_DEV\" = \"true\" ] && yarn run link-source && yalc add --link react-on-rails) || true"

Developers set REACT_ON_RAILS_DEV=true in their local environment.

---

🐛 Potential Issues

1. Silent Failures: The || exit 0 (or || true in revised version) means all errors are silently ignored. While this prevents installation failures, it makes debugging difficult when the script should run but fails.

2. Script Logic: The current implementation test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails has this flow:

   • If yarn.lock exists, continue to the yarn run link-source command
   • If yarn.lock doesn't exist, exit with 0 (success)
   • This is correct logic, but the detection method itself is flawed (see above)

---

📋 Missing Requirements (per CLAUDE.md)

Based on the project's mandatory guidelines, the following are required before merging:

❌ 1. Tests

• No tests added to verify the preinstall behavior
• Should add integration tests simulating both development and production installations

❌ 2. Documentation

• PR description's Summary section is incomplete (still has template text)
• PR checklist items are unchecked
• Consider adding a comment in the code explaining the detection logic

❌ 3. CHANGELOG

• CHANGELOG.md entry is missing (required per CLAUDE.md)
• Recommendation: Add entry describing this fix under "Unreleased" or next version

✅ 4. Linting

• The change to package.json should pass linting, but please run bundle exec rubocop and rake lint to verify

---

🔒 Security Considerations

✅ No major security concerns:

• The script executes shell commands, which is inherent to npm lifecycle scripts
• No user input is processed
• The || exit 0 prevents malicious exit codes from blocking installation

---

⚡ Performance Considerations

✅ Positive impact: This change will improve installation performance for end-users by skipping unnecessary development operations (yarn && yalc publish can take significant time).

---

🎯 Recommendations Summary

Must Fix (High Priority)

1. Improve detection method - Replace test -f yarn.lock with a more reliable check (see options above)
2. Add CHANGELOG entry - Required per project guidelines
3. Complete PR description - Fill out Summary section and checklist

Should Fix (Medium Priority)

4. Add tests - Verify behavior in dev and client installation contexts
5. Add inline comment - Explain the detection logic for maintainers

Consider

6. Error handling - Consider logging when the script is skipped vs. when it fails (for debugging)

---

🧪 Testing Suggestion

To verify the fix works correctly:

# Test 1: Development environment (should run link-source)
cd /path/to/react_on_rails/react_on_rails_pro
yarn install --force
# Should see yalc output

# Test 2: Simulate client installation (should NOT run link-source)
mkdir /tmp/test-client && cd /tmp/test-client
npm init -y
# Adjust package.json to install from GitHub or local path
npm install
# Should NOT see yalc output or errors

---

💡 Overall Assessment

Direction: ✅ Correct - This PR addresses a real issue
Implementation: ⚠️ Needs improvement - Detection method may not work as intended
Completeness: ⚠️ Missing docs/tests/CHANGELOG

Recommendation: Please test the detection logic in a real client installation scenario and consider the alternative detection methods. Also ensure all required documentation is added.

---

Thank you for your contribution! Let me know if you need help testing the fix or implementing the suggestions.

[alexeyr-ci2]

Solved in #1873.

[claude]

Code Review Summary

Thank you for identifying and working on this issue! However, I notice that PR #1873 has already solved this problem with a superior implementation.

---

Current Solution (PR #1873)

The repository now uses a Node.js script (script/preinstall.js) instead of shell commands that checks if the package is installed inside node_modules.

This approach is much more reliable than the yarn.lock check because:

1. Accurate Detection: Checks if the package is installed inside node_modules (production/client use) vs. the development monorepo
2. Cross-Platform: Works on Windows, macOS, and Linux without shell-specific syntax
3. Clear Intent: The code is self-documenting and easier to maintain
4. Fewer Edge Cases: Does not rely on file existence that could vary across environments

---

Issues with This PR Approach

The test -f yarn.lock method has several reliability problems:

1. False Positives - Client projects often have their own yarn.lock file, so the script would incorrectly run in client installations

2. Working Directory Assumption - The check depends on which directory npm/yarn uses as cwd during preinstall, which can vary between package managers and installation contexts

3. Monorepo Confusion - Monorepos may have multiple yarn.lock files at different levels, so the check does not reliably distinguish dev vs. production contexts

---

Additional Concerns (Per CLAUDE.md Requirements)

Even if this approach were viable, the PR is missing mandatory requirements:

• ❌ CHANGELOG entry (required before every commit per CLAUDE.md)
• ❌ Tests to verify installation behavior
• ❌ PR description completion (Summary section still has template text)
• ❌ Documentation explaining the new behavior

---

Recommendation

This PR should be closed in favor of the existing solution in PR #1873.

The preinstall.js approach is more reliable, easier to understand and maintain, already implemented and tested, and cross-platform compatible.

Thank you for bringing attention to this issue! While this particular implementation will not be merged, your identification of the problem was valuable and has already been addressed.

---

Status: ❌ Request to Close (superseded by #1873)

[claude]

Code Review: PR #1884

Thank you for working on this issue! I've reviewed the PR and have some important findings to share.

🚨 Critical Issue: PR is Outdated

The problem this PR attempts to solve has already been addressed in a better way on the master branch.

The PR diff shows a change to an inline bash script in package.json:

"preinstall": "test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails"

However, the current master branch (react_on_rails_pro/package.json:107) already uses a Node.js script:

"preinstall": "node ./script/preinstall.js"

This preinstall.js script (react_on_rails_pro/script/preinstall.js) implements a more robust solution:

1. ✅ Better detection method: Checks if __dirname contains 'node_modules' rather than looking for yarn.lock
2. ✅ More reliable: The node_modules check works regardless of which package manager is used (yarn, npm, pnpm)
3. ✅ Cross-platform: Pure Node.js without shell-specific syntax
4. ✅ Better error handling: Try-catch with informative error messages
5. ✅ Graceful degradation: Exits with code 0 even when link steps fail

Analysis of the Proposed Approach

The yarn.lock detection method has several limitations:

1. False negatives: Won't work if the project uses npm (package-lock.json) or pnpm (pnpm-lock.yaml)
2. Less semantic: Checking for a lockfile doesn't directly indicate whether it's a development or production install
3. Shell compatibility: The test -f syntax might behave differently across platforms

Recommendation

This PR should be closed as the issue has already been resolved with a superior implementation in commit that introduced react_on_rails_pro/script/preinstall.js.

Additional Notes

• The PR checklist items (tests, docs, CHANGELOG) were not completed
• The PR description still contains template placeholder text
• If you encountered this issue recently, please verify you're on the latest master branch

For Maintainers

Please confirm if the preinstall.js approach is the intended solution, and if so, this PR can be closed as already fixed.

---

Code Style Notes (per CLAUDE.md):

• ✅ The existing preinstall.js follows best practices
• ✅ Proper error handling is implemented
• ✅ Cross-platform compatibility maintained

[claude]

PR Review: Run "link-source" script in development environment only

Overview

This PR aims to prevent the link-source script from running when the package is installed in client projects (production use), limiting it to development environments only.

Critical Issue: PR Appears to Target Outdated Code

The PR diff shows changes to an inline shell script in package.json, but the current codebase already uses a more robust script/preinstall.js file. The current implementation (in script/preinstall.js:6-11) already solves this problem by checking if the package is inside node_modules:

const inNodeModules = __dirname.split(path.sep).includes('node_modules');

if (inNodeModules) {
  console.log('preinstall: running inside node_modules — skipping link steps.');
  process.exit(0);
}

This is a more reliable approach than checking for yarn.lock because:

1. More accurate detection: Checking node_modules in the path directly identifies when the package is installed as a dependency
2. Package manager agnostic: Works with npm, yarn, pnpm, etc.
3. No false positives: A client project could have a yarn.lock file, which would incorrectly trigger dev mode with the PR's approach

Issues with the PR's Proposed Approach

The PR suggests using test -f yarn.lock || exit 0:

Problems:

1. Inverted logic risk: This exits with 0 (success) when yarn.lock is NOT found, but the subsequent commands would fail anyway if needed tools aren't present
2. Client projects with yarn: If a client uses Yarn and has a yarn.lock in their project root, this check would incorrectly try to run dev-only commands
3. Silent failures: The || exit 0 pattern can hide legitimate errors
4. Platform compatibility: Shell conditionals in npm scripts can behave differently across platforms (Windows vs Unix)

Recommendation

It appears this PR may have been created against an older version of the code. The current implementation in script/preinstall.js already provides a better solution to this problem.

Suggested Actions:

1. Verify the PR is based on the latest code - The current main branch already has script/preinstall.js which solves this issue
2. If the issue still exists, please:
   • Provide specific reproduction steps showing the current implementation failing
   • Include test cases demonstrating the problem
   • Rebase the PR against the latest main branch
3. Consider closing this PR if the current implementation already solves the problem

Additional Notes from CLAUDE.md

Per the project guidelines:

• ✅ The change is minimal (1 line)
• ❌ Missing test coverage for the new behavior
• ❌ CHANGELOG not updated
• ❌ Documentation not updated
• ⚠️ PR checklist items not checked off

Questions for the Author

1. Can you reproduce the issue with the current script/preinstall.js implementation?
2. What specific scenario causes the current code to fail?
3. Have you tested your changes with both development installs and production (client project) installs?

---

Summary: The current codebase already has a more robust solution to this problem. Please verify if this PR is still needed and if so, rebase against the latest code and provide test cases demonstrating the issue.

[claude]

Code Review: PR #1884

Thank you for working on this issue! I've reviewed the changes and have some important feedback.

Critical Issue: PR Appears Outdated

The main branch already has a more robust solution that supersedes this PR's approach. The current master branch uses script/preinstall.js (introduced recently) which provides better cross-platform compatibility and error handling than the shell-based approach in this PR.

Current main branch solution (react_on_rails_pro/package.json:107):

"preinstall": "node ./script/preinstall.js"

The script/preinstall.js file:

• ✅ Detects if running inside node_modules (more reliable than checking for yarn.lock)
• ✅ Cross-platform compatible (works on Windows, Linux, macOS)
• ✅ Proper error handling with try/catch
• ✅ Graceful fallback without failing the installation

Issues with the PR's Approach

1. Unreliable Detection Method

test -f yarn.lock || exit 0

This approach has several problems:

• False positives: A yarn.lock file could exist in client projects, causing the script to run when it shouldn't
• False negatives: Development environments might not always have yarn.lock in the exact directory where the script runs
• Not the right indicator: The presence of a lockfile doesn't reliably indicate whether this is a development vs. production/consumer installation

2. Platform Compatibility
The test -f command is a shell built-in that may behave differently across shells (bash, zsh, sh, fish) and doesn't work on Windows without WSL/Git Bash.

3. Error Handling
The inline shell approach makes it harder to provide meaningful error messages to developers when things go wrong.

Recommendation

This PR should be closed as the issue it addresses has already been solved more robustly in the main branch via script/preinstall.js.

If you'd like to contribute further improvements:

1. Rebase against current master to see the existing solution
2. Test the current solution to verify it solves issue Don't run preinstall hook of node renderer package on installed as a dependency #1883
3. If there are still issues with the current preinstall.js, open a new PR with improvements to that file

Why script/preinstall.js is Better

The check __dirname.split(path.sep).includes('node_modules') is more reliable because:

• It directly detects if the package is installed as a dependency (in node_modules)
• Works consistently across all platforms (Node.js handles path separators)
• Clearer intent and easier to maintain

Additional Notes

Per the repository's CLAUDE.md:

• ✅ The PR checklist items (tests, documentation, CHANGELOG) have not been completed
• ⚠️ The PR description template wasn't fully filled out
• ⚠️ No tests were added to verify the fix works correctly

---

Verdict: Recommend closing this PR in favor of the existing script/preinstall.js solution already in master. Please verify the current solution addresses your use case, and if not, open a new issue with specific scenarios that still fail.

[claude]

Code Review for PR #1884

Summary

This PR attempts to fix an issue where the link-source script runs during package installation in client projects, which should only happen in the development environment.

Critical Issue: PR Appears Outdated ⚠️

The current codebase already has a better solution implemented.

The PR diff shows:

"preinstall": "test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails"

However, the current master branch already has a more robust solution:

"preinstall": "node ./script/preinstall.js"

The script/preinstall.js file (line 6-11) checks if the package is installed inside node_modules and skips linking steps if so:

const inNodeModules = __dirname.split(path.sep).includes('node_modules');

if (inNodeModules) {
  console.log('preinstall: running inside node_modules — skipping link steps.');
  process.exit(0);
}

Issues with the Proposed Approach

1. Less Reliable Detection: Checking for yarn.lock is not a reliable way to detect development vs production environments:

   • Client projects may have yarn.lock files
   • The lock file check doesn't actually indicate whether we're in the package's development environment
   • It could cause false positives/negatives

2. Cross-Platform Issues: The shell command test -f yarn.lock || exit 0 is not portable to Windows without additional tooling

3. Worse Error Handling: The proposed approach exits silently on errors, making debugging harder

Why the Current Solution is Better

The existing preinstall.js approach:

• ✅ More reliable: Detects actual installation context (inside node_modules vs development)
• ✅ Cross-platform: Pure Node.js works on all platforms
• ✅ Better error handling: Logs errors but doesn't fail installation (line 28-30)
• ✅ More maintainable: Clear logic in a dedicated script file
• ✅ Better logging: Provides informative console messages

Recommendation

This PR should be closed as the issue it attempts to fix has already been resolved with a superior solution in the current codebase.

If you're experiencing issues with the current implementation, please:

1. Verify you're testing against the latest master branch
2. Open a new issue describing the specific problem with the current preinstall.js approach
3. Ensure the script/preinstall.js file is included in the published package (it's in the files array on line 104)

Additional Notes from CLAUDE.md

The PR also doesn't address the checklist items:

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

These would be required for any future improvements to this functionality.

---

Review generated by Claude Code following repository guidelines

[claude]

PR Review: Run "link-source" script in development environment only

Thank you for this contribution! However, after reviewing the code, I've identified that this PR appears to be outdated and the problem has already been solved in a better way.

Current State vs. PR Changes

What this PR proposes:

"preinstall": "test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails"

What's already in master:

"preinstall": "node ./script/preinstall.js"

The codebase already has a script/preinstall.js file (react_on_rails_pro/package.json:107) that implements a superior solution to the problem you're trying to solve.

Why the Current Solution is Better

The existing script/preinstall.js approach:

1. ✅ Cross-platform compatibility: Works on Windows, Linux, and macOS without requiring shell-specific syntax
2. ✅ More robust detection: Checks if running inside node_modules rather than just checking for a lockfile

   const inNodeModules = __dirname.split(path.sep).includes('node_modules');

3. ✅ Better error handling: Properly catches and logs errors without failing the installation
4. ✅ Clearer intent: The code is self-documenting and easier to understand
5. ✅ More maintainable: JavaScript is easier to extend than complex shell one-liners

Issues with the PR Approach

1. ❌ Lockfile detection is unreliable: A project might use npm or pnpm instead of yarn, or have no lockfile
2. ❌ Shell compatibility: The test -f command may behave differently across shells and platforms
3. ❌ Silent failures: The || exit 0 approach makes debugging harder
4. ❌ Hard to maintain: Complex shell one-liners are difficult to read and extend

Recommendation

This PR should be closed as the issue has been resolved more comprehensively in commit that introduced script/preinstall.js.

If you're experiencing issues with the current preinstall behavior, please:

1. Ensure you have the latest master branch
2. Verify that script/preinstall.js exists
3. Test the current implementation
4. Open a new issue if problems persist

Additional Notes

Per the CLAUDE.md guidelines:

• The PR checklist items (tests, documentation, CHANGELOG) were not completed
• The summary section still contains placeholder text

Thank you for contributing! While this particular change isn't needed, your attention to the development workflow issues is appreciated.

[claude]

Code Review - PR #1884

Overview

This PR attempts to prevent the link-source script from running when the package is installed in a client project (non-development environment) by checking for the existence of a yarn.lock file.

⚠️ Critical Issues

1. Outdated PR - Already Superseded

The current master branch already has a better solution implemented via script/preinstall.js (added in a previous commit). This script:

• Checks if the package is installed inside node_modules using __dirname.split(path.sep).includes('node_modules')
• Is more reliable and cross-platform than checking for yarn.lock
• Properly handles errors without silently failing

Your PR changes are targeting an older version of the code that no longer exists in master.

2. Flawed Detection Logic

The proposed approach (test -f yarn.lock || exit 0) has several problems:

Problem A: Unreliable Detection

• Checking for yarn.lock is not a reliable way to detect development vs. production environments
• Client projects can (and often do) have their own yarn.lock files
• This would cause the link script to run in client projects if they use Yarn, defeating the purpose

Problem B: Shell Command Portability

• The test -f command may not work consistently across all platforms (Windows compatibility concerns)
• The current Node.js-based solution in preinstall.js is more portable

3. Silent Failure Semantics

The construct test -f yarn.lock || exit 0 means:

• If yarn.lock exists → continue to run link commands
• If yarn.lock doesn't exist → exit 0 (success, skip remaining commands)

This is the opposite of what the commit message describes. The second commit mentions "don't silently ignore errors" but the current implementation still allows silent failures.

📋 Comparison with Current Master

Current Master (preinstall.js):

const inNodeModules = __dirname.split(path.sep).includes('node_modules');
if (inNodeModules) {
  console.log('preinstall: running inside node_modules — skipping link steps.');
  process.exit(0);
}

Your PR (obsolete approach):

test -f yarn.lock || exit 0; yarn run link-source && yalc add --link react-on-rails

The Node.js approach is:

• ✅ More reliable (directly checks installation location)
• ✅ Better error handling with try-catch
• ✅ Cross-platform compatible
• ✅ Provides helpful console messages
• ✅ Gracefully handles missing dependencies

🎯 Recommendations

1. Close this PR - The issue is already solved in master with a better implementation
2. Verify the current solution - Test that script/preinstall.js works correctly in your use case
3. Update your branch - Pull latest master to get the current solution
4. If issues persist - Open a new issue describing what doesn't work with the current preinstall.js implementation

📝 Additional Notes

Missing from PR (as noted in checklist):

• ❌ No tests added/updated
• ❌ No documentation updates
• ❌ No CHANGELOG entry

Code Quality Observations:

• The current master implementation follows better practices with proper error handling
• The Node.js script approach is more maintainable than inline shell commands
• The current solution has informative logging for debugging

🔍 Security & Performance

• No security concerns with either approach
• Performance impact is negligible (only runs during install)
• Current master implementation is actually more performant (no external test process spawn)

---

Verdict: This PR should be closed as the problem is already solved more elegantly in the master branch. Please sync with master and verify the existing solution meets your needs.

[AbanoubGhadban]

Solved in #1873.