Update react-on-rails import to /client as needed
#1703
Conversation
WalkthroughThis pull request updates various documentation and example files to reflect a more explicit usage of client-specific import paths for ReactOnRails. The changes include updates to import references in documentation, guides, code examples, and dummy application views. Additionally, a method in the DevTestsGenerator was modified for managing local dependencies, test commands were updated in CONTRIBUTING.md, and the conversion script was adjusted to use regular expressions for gem version matching. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevTestsGenerator
participant PackageJson
User->>DevTestsGenerator: Run make_react_on_rails_dependency_local
DevTestsGenerator->>PackageJson: Read package.json content
PackageJson-->>DevTestsGenerator: Return file content
DevTestsGenerator->>PackageJson: Update react-on-rails dependency (local via yalc)
DevTestsGenerator->>PackageJson: Write updated content back to package.json
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/update-client-imports
branch
2 times, most recently
from
7210fe2 to
745c1f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/generators/react_on_rails/base_generator.rb (1)
79-93: 💡 Verification agent🧩 Analysis chain
Verify consistency with import path changes.
This generator change supports the overall PR objective of updating react-on-rails imports, but I should verify whether the generated code templates also reflect the updated import paths (from 'react-on-rails' to 'react-on-rails/client') mentioned in the PR summary.
🏁 Script executed:
#!/bin/bash # Check if the templates used by this generator have been updated to use the new import path # Look for import statements in JavaScript templates echo "Checking for import statements in JavaScript templates:" rg "from\s+['\"]react-on-rails['\"]" lib/generators/react_on_rails/templates/ -g "*.js*" -g "*.ts*" # Look for any react-on-rails imports in other files echo "Checking other template files for react-on-rails imports:" rg "react-on-rails" lib/generators/react_on_rails/templates/ -g "!*.rb"Length of output: 825
Update Generator Template Import Paths for Consistency
We verified that while
registration.js.ttcorrectly uses the updated import path (react-on-rails/client), the filelib/generators/react_on_rails/templates/base/base/app/javascript/packs/server-bundle.jsstill imports fromreact-on-rails. Please update the import inserver-bundle.jsto usereact-on-rails/clientto ensure all generated templates align with the updated PR objective.
🧹 Nitpick comments (2)
script/convert (2)
16-16: Improved regex pattern for matching shakapacker gem versionThe change from a fixed string to a regular expression pattern (
/gem "shakapacker", "[^"]*"/) makes the script more robust by allowing it to match any version of the shakapacker gem before replacing it with version 6.6.0.Consider adding a comment explaining why this specific version (6.6.0) is being used to help future maintainers understand the version selection reasoning.
16-22: Ensure consistent version management across the codebaseWhile the script now handles version patterns more flexibly, it's worth noting that hardcoded version numbers (6.6.0) appear in multiple places. This could make future version updates more error-prone.
Consider defining the version number as a variable at the top of the script to ensure consistency:
#!/usr/bin/env ruby # frozen_string_literal: true +SHAKAPACKER_VERSION = "6.6.0" + def gsub_file_content(path, old_content, new_content) path = File.expand_path(path, __dir__) content = File.binread(path) content.gsub!(old_content, new_content) File.binwrite(path, content) end old_config = File.expand_path("../spec/dummy/config/shakapacker.yml", __dir__) new_config = File.expand_path("../spec/dummy/config/webpacker.yml", __dir__) File.rename(old_config, new_config) -gsub_file_content("../Gemfile.development_dependencies", /gem "shakapacker", "[^"]*"/, 'gem "shakapacker", "6.6.0"') +gsub_file_content("../Gemfile.development_dependencies", /gem "shakapacker", "[^"]*"/, "gem \"shakapacker\", \"#{SHAKAPACKER_VERSION}\"") # The below packages don't work on the oldest supported Node version and aren't needed there anyway gsub_file_content("../package.json", /"knip": "[^"]*",/, "") gsub_file_content("../package.json", %r{"@arethetypeswrong/cli": "[^"]*",}, "") -gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') +gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, "\"shakapacker\": \"#{SHAKAPACKER_VERSION}\",")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CONTRIBUTING.md(1 hunks)docs/api/javascript-api.md(1 hunks)docs/guides/how-to-use-different-files-for-client-and-server-rendering.md(2 hunks)docs/javascript/code-splitting.md(3 hunks)lib/generators/react_on_rails/base_generator.rb(1 hunks)lib/generators/react_on_rails/dev_tests_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/packs/registration.js.tt(1 hunks)script/convert(1 hunks)spec/dummy/app/views/pages/client_side_hello_world.html.erb(1 hunks)spec/dummy/app/views/pages/server_side_hello_world.html.erb(1 hunks)spec/dummy/app/views/pages/server_side_redux_app.html.erb(1 hunks)spec/dummy/app/views/pages/server_side_redux_app_cached.html.erb(1 hunks)spec/dummy/app/views/pages/xhr_refresh.html.erb(1 hunks)spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
- docs/guides/how-to-use-different-files-for-client-and-server-rendering.md
- lib/generators/react_on_rails/templates/base/base/app/javascript/packs/registration.js.tt
- lib/generators/react_on_rails/dev_tests_generator.rb
- spec/dummy/app/views/pages/server_side_redux_app_cached.html.erb
- spec/dummy/app/views/pages/server_side_redux_app.html.erb
- spec/dummy/app/views/pages/server_side_hello_world.html.erb
- spec/dummy/app/views/pages/client_side_hello_world.html.erb
- spec/dummy/app/views/pages/xhr_refresh.html.erb
- docs/javascript/code-splitting.md
- docs/api/javascript-api.md
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (oldest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (4)
CONTRIBUTING.md (1)
219-219: Update Reflects New Testing WorkflowThe updated command instructions now correctly reference the new test tasks (
rake run_rspec:shakapacker_examples_basicandrake run_rspec:shakapacker_examples). The note explaining that these commands will generate a huge number of files reinforces the importance of excluding the/gen-examplesdirectory from your IDE, which is very useful to prevent potential performance issues. Overall, the changes are clear, consistent with the PR objectives, and enhance the documentation effectively.lib/generators/react_on_rails/base_generator.rb (2)
79-81: Good addition of a utility method for local development.This method provides a convenient hook that could be overridden in subclasses or for local development purposes to use a local version of the react-on-rails package instead of the published one.
85-87: Clean implementation of local development support.The conditional logic integrates well with the existing code. This change allows developers to use a local version of the react-on-rails package (from .yalc directory) when developing or testing changes to the package itself. This is particularly useful for maintainers who need to test compatibility with the generator before publishing changes.
script/convert (1)
1-28:❓ Verification inconclusive
Relationship with PR objectives needs verification
The PR is titled "Update
react-on-railsimport to/clientas needed", but this script doesn't directly modify import paths. It focuses on shakapacker version management and webpack configuration changes.Let's check if this script is part of a larger update process that might indirectly support the import path changes:
🏁 Script executed:
#!/bin/bash # Description: Check if this script is part of a process that updates import paths # Look for other files mentioning import path changes echo "Files containing both 'react-on-rails' and '/client' references:" rg -l "react-on-rails/client" | head -n 10 # Check if this script is called from other scripts that might be doing the import updates echo -e "\nChecking if this script is referenced in other files:" rg -l "script/convert" --type-not ruby | head -n 5Length of output: 931
Clarification Required: Confirm Conversion Script’s Role in PR Objectives
- The
script/convertfile currently focuses on renaming configuration files, updating shakapacker-related versions in the Gemfile and package.json files, and adjusting webpack configuration calls.- No modifications targeting
react-on-railsimport paths (i.e. updating them to/client) are evident here.- A repository search shows that while several files include
/clientreferences forreact-on-rails, this script is only referenced in documentation (CONTRIBUTING.md) and does not directly perform any import updates.- Please clarify whether:
- This script is meant to work in tandem with other changes that handle
react-on-railsimport updates, or- The PR title/description needs updating to reflect the actual scope of changes.
|
|
||
| # The below packages don't work on the oldest supported Node version and aren't needed there anyway | ||
| gsub_file_content("../package.json", /"knip": "[^"]*",/, "") | ||
| gsub_file_content("../package.json", %r{"@arethetypeswrong/cli": "[^"]*",}, "") | ||
|
|
||
| gsub_file_content("../spec/dummy/package.json", '"shakapacker": "8.0.0",', '"shakapacker": "6.6.0",') | ||
| gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhanced flexibility for package.json version matching
Similar to the Gemfile change, this improves the script by using a regex pattern to match any version of shakapacker in the package.json file before replacing it with a specific version.
Let's verify that both the gem and npm package versions are aligned at 6.6.0:
🏁 Script executed:
#!/bin/bash
# Description: Verify that both the gem version in Gemfile.development_dependencies
# and npm package version in spec/dummy/package.json are set to 6.6.0
echo "Checking Gemfile.development_dependencies for shakapacker version:"
cat Gemfile.development_dependencies | grep -E 'gem "shakapacker"'
echo -e "\nChecking spec/dummy/package.json for shakapacker version:"
cat spec/dummy/package.json | grep -E '"shakapacker":'Length of output: 454
Version mismatch detected – update the hardcoded version
The regex pattern in the conversion script is sound for flexible matching, but the replacement value is outdated. The command in script/convert (line 22) currently sets shakapacker to "6.6.0", whereas both the Gemfile and spec/dummy/package.json verify that shakapacker is now at "8.0.0". Please update the replacement value accordingly.
- File:
script/convert(line 22) - Diff Suggestion:
- gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') + gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "8.0.0",')
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') | |
| gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "8.0.0",') |
alexeyr-ci
force-pushed
the
alexeyr/update-client-imports
branch
from
745c1f3 to
457da34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/generators/react_on_rails/dev_tests_generator.rb (3)
60-65: Good addition of a dependency management method, but line length exceeds limit.The new method effectively complements the existing
add_yarn_relative_install_script_in_package_jsonmethod by replacing the version specification in package.json to use a local yalc-linked version. This is a useful addition for local development workflows.To fix the line length issue reported by Rubocop (134/120 characters), consider breaking up the regex line:
- new_client_package_json_contents = contents.gsub(/"react-on-rails": "[^"]*"/, '"react-on-rails": "link:.yalc/react-on-rails"') + pattern = /"react-on-rails": "[^"]*"/ + replacement = '"react-on-rails": "link:.yalc/react-on-rails"' + new_client_package_json_contents = contents.gsub(pattern, replacement)🧰 Tools
🪛 RuboCop (1.73)
[convention] 63-63: Line is too long. [134/120]
(Layout/LineLength)
🪛 GitHub Actions: Lint JS and Ruby
[warning] 63-63: Rubocop: Line is too long. [134/120]
60-65: Consider adding error handling for file operations.The method currently doesn't handle cases where the package.json file might not exist or where the react-on-rails dependency might be missing or formatted differently.
def make_react_on_rails_dependency_local package_json = File.join(destination_root, "package.json") + return unless File.exist?(package_json) contents = File.read(package_json) - new_client_package_json_contents = contents.gsub(/"react-on-rails": "[^"]*"/, '"react-on-rails": "link:.yalc/react-on-rails"') + pattern = /"react-on-rails": "[^"]*"/ + replacement = '"react-on-rails": "link:.yalc/react-on-rails"' + new_client_package_json_contents = contents.gsub(pattern, replacement) + # Verify the replacement happened + if contents == new_client_package_json_contents + puts "Warning: Could not find 'react-on-rails' dependency in package.json" + end File.open(package_json, "w+") { |f| f.puts new_client_package_json_contents } end🧰 Tools
🪛 RuboCop (1.73)
[convention] 63-63: Line is too long. [134/120]
(Layout/LineLength)
🪛 GitHub Actions: Lint JS and Ruby
[warning] 63-63: Rubocop: Line is too long. [134/120]
60-65: Add documentation comment explaining the method's purpose.A documentation comment would help developers understand the method's purpose and how it relates to the overall workflow.
+# Modifies the package.json to use a local version of react-on-rails via yalc +# This works in conjunction with the postinstall script added by add_yarn_relative_install_script_in_package_json def make_react_on_rails_dependency_local package_json = File.join(destination_root, "package.json") contents = File.read(package_json) new_client_package_json_contents = contents.gsub(/"react-on-rails": "[^"]*"/, '"react-on-rails": "link:.yalc/react-on-rails"') File.open(package_json, "w+") { |f| f.puts new_client_package_json_contents } end🧰 Tools
🪛 RuboCop (1.73)
[convention] 63-63: Line is too long. [134/120]
(Layout/LineLength)
🪛 GitHub Actions: Lint JS and Ruby
[warning] 63-63: Rubocop: Line is too long. [134/120]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.md(1 hunks)lib/generators/react_on_rails/dev_tests_generator.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/app/javascript/packs/registration.js.tt(1 hunks)script/convert(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- script/convert
- CONTRIBUTING.md
- lib/generators/react_on_rails/templates/base/base/app/javascript/packs/registration.js.tt
🧰 Additional context used
🪛 RuboCop (1.73)
lib/generators/react_on_rails/dev_tests_generator.rb
[convention] 63-63: Line is too long. [134/120]
(Layout/LineLength)
🪛 GitHub Actions: Lint JS and Ruby
lib/generators/react_on_rails/dev_tests_generator.rb
[warning] 63-63: Rubocop: Line is too long. [134/120]
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (oldest)
alexeyr-ci
force-pushed
the
alexeyr/update-client-imports
branch
from
457da34 to
88e3390
Compare
alexeyr-ci
force-pushed
the
alexeyr/update-client-imports
branch
from
88e3390 to
72040b5
Compare
Summary
Update documentation and examples after #1697.
Pull Request checklist
- [ ] Add/update test to cover these changes- [ ] Update CHANGELOG fileThis change is
Summary by CodeRabbit
Documentation
Chores