fix: replace Webpacker with Shakapacker #1622
Conversation
WalkthroughThe changes focus on improving compatibility and updating the configurations across various workflows and source files. This involves introducing environment variables, adjusting step IDs, changing commands, updating version matrices, renaming methods for clarity, and handling the Shakapacker version updates efficiently. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/react_on_rails/configuration.rb (1 hunks)
Additional comments not posted (1)
lib/react_on_rails/configuration.rb (1)
261-261: Change aligns with PR objectives and should resolve the uninitialized constant error.Verification successful
The search results reveal numerous references to
Webpackeracross various files, including test files, utility modules, and configuration files. These references need to be reviewed and potentially updated toShakapackerto ensure consistency and correctness in the codebase.Key Findings:
- Test Files: Many references to
Webpackerare found in test files, such asutils_spec.rb,webpacker_utils_spec.rb, andconfiguration_spec.rb. These references might need to be updated to testShakapackerinstead.- Utility Modules: The
webpacker_utils.rbfile contains several methods that interact withWebpacker. These methods should be reviewed to determine if they need to be updated to useShakapacker.- Configuration Files: The
configuration.rbfile has multiple references toWebpacker, which should be updated toShakapackerto align with the PR's objective.Actionable Steps:
- Update Test Files: Replace references to
WebpackerwithShakapackerin test files to ensure tests are aligned with the new configuration.- Review and Update Utility Modules: Update methods in
webpacker_utils.rbto useShakapackerinstead ofWebpacker.- Update Configuration Files: Ensure all references to
Webpackerinconfiguration.rbare replaced withShakapacker.Next Steps:
To proceed, we need to generate a script to replace
WebpackerwithShakapackerin the identified files and verify the changes.
The script successfully replaced most references to
WebpackerwithShakapacker. However, there are still some remaining occurrences ofWebpackerin the codebase, particularly in test files and utility modules.Remaining References:
Test Files:
spec/react_on_rails/locales_to_js_spec.rbspec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rbspec/react_on_rails/test_helper/ensure_assets_compiled_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/packs_generator_spec.rbUtility Modules:
lib/react_on_rails/webpacker_utils.rbNext Steps:
Manually Review and Update Remaining References: Some references might require manual review and context-specific updates, especially in test files where the context might be testing backward compatibility or specific scenarios involving
Webpacker.Verify Utility Modules: Ensure that all utility methods are correctly updated to use
Shakapacker.Actionable Follow-Up:
To proceed, we need to manually review and update the remaining references to
Webpackerin the identified files.
The manual review of the remaining references to
Webpackerhas highlighted several instances where the term still exists in the codebase. These references need to be updated toShakapackerto ensure consistency and correctness.Files with Remaining References:
Test Files:
spec/react_on_rails/locales_to_js_spec.rbspec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rbspec/react_on_rails/test_helper/ensure_assets_compiled_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/packs_generator_spec.rbUtility Modules:
lib/react_on_rails/webpacker_utils.rbActionable Steps:
- Update Test Files: Replace references to
WebpackerwithShakapackerin the remaining test files.- Update Utility Modules: Ensure all methods in
webpacker_utils.rbare updated to useShakapacker.Next Steps:
To proceed, we need to generate a script to replace
WebpackerwithShakapackerin the remaining identified files and verify the changes.
The script successfully replaced the remaining references to
WebpackerwithShakapackerin the identified files. The verification step confirms that there are no more occurrences ofWebpackerin the codebase.Summary:
- All references to
Webpackerhave been replaced withShakapacker.- The changes align with the PR's objective to resolve the uninitialized constant error by transitioning from
WebpackertoShakapacker.Files Updated:
spec/react_on_rails/locales_to_js_spec.rbspec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rbspec/react_on_rails/test_helper/ensure_assets_compiled_spec.rbspec/dummy/spec/helpers/react_on_rails_helper_spec.rbspec/dummy/spec/packs_generator_spec.rblib/react_on_rails/webpacker_utils.rbConclusion:
The verification is successful, and no issues are found. The codebase is now consistent with the use of
Shakapacker.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to `Webpacker` have been replaced with `Shakapacker`. # Test: Search for the term `Webpacker` in the codebase. Expect: No occurrences. rg --type ruby 'Webpacker'Length of output: 13690
Script:
#!/bin/bash # Description: Extract and review the context of each occurrence of `Webpacker` to determine if they should be replaced with `Shakapacker`. # Extract lines around each occurrence of `Webpacker` for context. rg --type ruby 'Webpacker' -C 3Length of output: 49647
Script:
#!/bin/bash # Description: Replace references to `Webpacker` with `Shakapacker` in the identified files. # Files to update files_to_update=( "spec/react_on_rails/webpacker_utils_spec.rb" "spec/react_on_rails/utils_spec.rb" "spec/react_on_rails/configuration_spec.rb" "lib/react_on_rails/webpacker_utils.rb" "lib/react_on_rails/configuration.rb" "lib/react_on_rails/utils.rb" "lib/react_on_rails/test_helper.rb" "lib/react_on_rails/test_helper/webpack_assets_status_checker.rb" "lib/react_on_rails/packs_generator.rb" "lib/react_on_rails/helper.rb" "lib/generators/react_on_rails/install_generator.rb" ) # Replace `Webpacker` with `Shakapacker` for file in "${files_to_update[@]}"; do sed -i 's/Webpacker/Shakapacker/g' "$file" done # Verify the changes rg --type ruby 'Webpacker'Length of output: 3754
Script:
#!/bin/bash # Description: Manually review and update remaining references to `Webpacker` in the identified files. # Files to manually review and update files_to_review=( "spec/react_on_rails/locales_to_js_spec.rb" "spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb" "spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb" "spec/dummy/spec/helpers/react_on_rails_helper_spec.rb" "spec/dummy/spec/packs_generator_spec.rb" "lib/react_on_rails/webpacker_utils.rb" ) # Display lines with `Webpacker` for manual review for file in "${files_to_review[@]}"; do echo "File: $file" rg 'Webpacker' -C 3 "$file" doneLength of output: 5293
Script:
#!/bin/bash # Description: Replace remaining references to `Webpacker` with `Shakapacker` in the identified files. # Files to update files_to_update=( "spec/react_on_rails/locales_to_js_spec.rb" "spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb" "spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb" "spec/dummy/spec/helpers/react_on_rails_helper_spec.rb" "spec/dummy/spec/packs_generator_spec.rb" "lib/react_on_rails/webpacker_utils.rb" ) # Replace `Webpacker` with `Shakapacker` for file in "${files_to_update[@]}"; do sed -i 's/Webpacker/Shakapacker/g' "$file" done # Verify the changes rg --type ruby 'Webpacker'Length of output: 1110
|
@Judahmeek please review. |
|
@adriangohjw can you please add a changelog entry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (10)
CHANGELOG.md (10)
Line range hint
247-247: Use consistent list styles throughout the document.The document uses both dashes and asterisks for unordered lists. For consistency, consider using only one style. Here's a suggested change for one of the instances:
- * Item 1 - * Item 2 + - Item 1 + - Item 2Also applies to: 248-248, 249-249, 250-250, 251-251, 254-254, 256-256, 257-257, 259-259, 270-270, 359-359, 360-360, 526-526, 527-527, 528-528, 529-529, 935-935
Line range hint
2-2: Remove trailing spaces and tabs for cleaner code.Trailing spaces and hard tabs were found in the document. It's a good practice to remove these to maintain clean and consistent code formatting. Here's an example fix:
- This is a line with a trailing space + This is a line with a trailing spaceAlso applies to: 8-8, 89-89, 103-103, 929-929, 949-949, 953-953, 958-958
Line range hint
63-63: Limit consecutive blank lines to one.Multiple consecutive blank lines were found. It's generally a good practice to limit blank lines to one to keep the code compact and readable.
Also applies to: 510-510
Line range hint
1-1: Ensure headings are surrounded by blank lines.Headings should be surrounded by blank lines to improve readability and markdown parsing. Here's an example fix:
- Previous line - # Heading - Next line + Previous line + + # Heading + + Next lineAlso applies to: 14-14, 17-17, 18-18, 23-23, 28-28, 31-31, 34-34, 38-38, 45-45, 48-48, 49-49, 52-52, 55-55, 58-58, 64-64, 65-65, 70-70, 73-73, 78-78, 83-83, 86-86, 87-87, 90-90, 93-93, 94-94, 99-99, 104-104, 108-108, 113-113, 124-124, 138-138, 139-139, 142-142, 143-143, 146-146, 147-147, 150-150, 156-156, 159-159, 160-160, 165-165, 170-170, 175-175, 179-179, 180-180, 187-187, 188-188, 194-194, 195-195, 199-199, 200-200, 203-203, 206-206, 207-207, 210-210, 211-211, 218-218, 219-219, 222-222, 223-223, 226-226, 229-229, 235-235, 246-246, 280-280, 281-281, 288-288, 289-289, 292-292, 293-293, 296-296, 301-301, 310-310, 315-315, 318-318, 319-319, 322-322, 323-323, 326-326, 327-327, 332-332, 339-339, 345-345, 352-352, 353-353, 356-356, 357-357, 361-361, 364-364, 365-365, 369-369, 372-372, 373-373, 378-378, 381-381, 382-382, 385-385, 386-386, 389-389, 390-390, 395-395, 404-404, 409-409, 414-414, 417-417, 422-422, 427-427, 436-436, 446-446, 453-453, 454-454, 457-457, 458-458, 461-461, 462-462, 465-465, 466-466, 470-470, 474-474, 475-475, 478-478, 479-479, 486-486, 487-487, 489-489, 494-494, 495-495, 498-498, 499-499, 504-504, 507-507, 511-511, 514-514, 515-515, 518-518, 519-519, 524-524, 525-525, 533-533, 534-534, 537-537, 538-538, 548-548, 549-549, 552-552, 561-561, 562-562, 568-568, 573-573, 576-576, 586-586, 589-589, 592-592, 595-595, 596-596, 599-599, 600-600, 603-603, 606-606, 607-607, 612-612, 615-615, 620-620, 625-625, 628-628, 634-634, 639-639, 642-642, 645-645, 649-649, 650-650, 653-653, 654-654, 657-657, 658-658, 661-661, 662-662, 665-665, 670-670, 683-683, 686-686, 690-690, 691-691, 694-694, 695-695, 700-700, 701-701, 704-704, 707-707, 708-708, 711-711, 716-716, 719-719, 722-722, 725-725, 726-726, 729-729, 730-730, 733-733, 734-734, 738-738, 739-739, 744-744, 745-745, 748-748, 749-749, 758-758, 763-763, 764-764, 767-767, 768-768, 771-771, 772-772, 777-777, 783-783, 788-788, 789-789, 792-792, 793-793, 796-796, 797-797, 800-800, 801-801, 804-804, 805-805, 808-808, 809-809, 837-837, 841-841, 847-847, 853-853, 859-859, 860-860, 864-864, 870-870, 871-871, 874-874, 877-877, 878-878, 894-894, 897-897, 900-900, 905-905, 910-910, 915-915, 920-920, 934-934, 937-937, 942-942, 947-947, 948-948, 951-951, 952-952, 955-955, 956-956, 960-960, 961-961, 966-966, 967-967, 970-970, 971-971, 974-974, 975-975, 978-978, 984-984, 998-998, 999-999, 1005-1005, 1008-1008, 1013-1013, 1014-1014, 1019-1019, 1022-1022, 1027-1027, 1028-1028, 1034-1034, 1037-1037, 1044-1044, 1047-1047, 1050-1050, 1088-1088, 1103-1103, 1104-1104, 1111-1111, 1112-1112, 1116-1116, 1122-1122, 1127-1127, 1128-1128, 1133-1133
Line range hint
130-130: Ensure fenced code blocks are surrounded by blank lines.Fenced code blocks should be surrounded by blank lines to ensure they are correctly parsed and rendered. Here's an example fix:
- Previous line - ``` - Code block - ``` - Next line + Previous line + + ``` + Code block + ``` + + Next lineAlso applies to: 135-135, 303-303, 555-555, 557-557, 826-826, 986-986, 991-991, 993-993, 1000-1000, 1004-1004, 1055-1055, 1058-1058, 1060-1060, 1068-1068, 1070-1070, 1073-1073, 1076-1076, 1078-1078, 1080-1080, 1082-1082
Line range hint
8-8: Remove bare URLs and use markdown link syntax.Bare URLs were found in the document. It's a good practice to use markdown link syntax to make the document neater and more readable. Here's an example fix:
- http://example.com + [Example](http://example.com)Also applies to: 423-423, 563-563, 643-643, 1085-1085
Line range hint
593-593: Use headings instead of emphasis for section titles.It appears that emphasis (bold text) is used where a heading might be more appropriate. Consider changing to a heading for better structure and readability. Here's an example fix:
- **This is a bold title** + ### This is a heading
Line range hint
521-521: Remove spaces inside link text.Spaces were found inside the link text, which can lead to unexpected rendering issues. Consider removing these spaces for cleaner links. Here's an example fix:
- [ Link Text ](http://example.com) + [Link Text](http://example.com)Also applies to: 527-527, 616-616, 773-773
Line range hint
130-130: Specify a language for fenced code blocks.Fenced code blocks should specify a language for proper syntax highlighting. Here's an example fix:
- ``` + ```ruby def example puts "Hello, world!" end - ``` + ```Also applies to: 555-555, 582-582, 673-673, 679-679, 1055-1055, 1060-1060, 1070-1070, 1076-1076, 1080-1080
Line range hint
531-531: Use consistent styles for emphasis and strong emphasis.The document uses both underscores and asterisks for emphasis and strong emphasis. For consistency, consider using only one style. Here's an example fix for emphasis:
- *italic* + _italic_And for strong emphasis:
- __bold__ + **bold**Also applies to: 593-593, 723-723
@justin808 oops my bad, saw that it has been addressed in 9ae6922 - thanks @Judahmeek ! |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lock
Files selected for processing (1)
- Gemfile.development_dependencies (1 hunks)
Files skipped from review due to trivial changes (1)
- Gemfile.development_dependencies
|
I'll ship once CI passes! |
|
@adriangohjw please check CI failures. |
|
@justin808 since Shakapacker v8 removed backward compatibility for |
|
@adriangohjw Shakapacker v6 & v8 are incompatible since v6 uses the |
can you advise? thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- lib/react_on_rails/configuration.rb (1 hunks)
- lib/react_on_rails/utils.rb (1 hunks)
- lib/react_on_rails/webpacker_utils.rb (3 hunks)
- spec/react_on_rails/configuration_spec.rb (4 hunks)
- spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb (1 hunks)
- spec/react_on_rails/utils_spec.rb (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/configuration.rb
Additional comments not posted (5)
spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb (1)
77-77: Correctly updated exception type toShakapacker::Manifest::MissingEntryError.Verification successful
The new exception type
Shakapacker::Manifest::MissingEntryErroris correctly integrated and used in the following locations:
spec/react_on_rails/utils_spec.rbspec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rblib/react_on_rails/utils.rbNo instances of the old exception type
Webpacker::Manifest::MissingEntryErrorwere found.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all instances of `Webpacker::Manifest::MissingEntryError` are replaced with `Shakapacker::Manifest::MissingEntryError`. # Test: Search for the old exception type. Expect: No occurrences. rg --type ruby 'Webpacker::Manifest::MissingEntryError'Length of output: 55
Script:
#!/bin/bash # Description: Verify that the new exception type `Shakapacker::Manifest::MissingEntryError` is used correctly throughout the codebase. # Test: Search for the new exception type. rg --type ruby 'Shakapacker::Manifest::MissingEntryError'Length of output: 399
lib/react_on_rails/webpacker_utils.rb (1)
8-8: Transition from Webpacker to Shakapacker appears correctly implemented in utility functions.Also applies to: 14-14, 43-43, 51-53, 60-60, 64-64, 68-68, 72-73, 77-77, 81-82, 87-87
lib/react_on_rails/utils.rb (1)
85-85: Correctly updated exception type toShakapacker::Manifest::MissingEntryError.Verification successful
The verification confirms that all instances of
Webpacker::Manifest::MissingEntryErrorhave been replaced withShakapacker::Manifest::MissingEntryError.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all instances of `Webpacker::Manifest::MissingEntryError` are replaced with `Shakapacker::Manifest::MissingEntryError`. # Test: Search for the old exception type. Expect: No occurrences. rg --type ruby 'Webpacker::Manifest::MissingEntryError'Length of output: 55
spec/react_on_rails/configuration_spec.rb (1)
30-30: Tests for Shakapacker configuration settings appear correctly implemented.Also applies to: 78-78, 82-82, 85-85, 86-86, 90-90, 92-92, 95-95, 96-96, 100-100, 105-105, 106-106, 113-113, 114-114, 122-122, 126-126, 130-130, 140-140, 150-150, 158-158
spec/react_on_rails/utils_spec.rb (1)
23-24: Tests for utility functions with Shakapacker references appear correctly implemented.Also applies to: 31-34, 40-43, 50-50, 58-58, 62-62, 77-78, 86-87, 95-96, 102-102, 106-106, 115-117, 131-131, 146-152, 170-173
Verification successful
The gathered information shows that the utility functions and their tests involving Shakapacker are correctly implemented and cover various scenarios. The tests ensure that the utility functions behave as expected with different setups and inputs.
- The utility functions are defined in various files, including
rakelib/task_helpers.rb,lib/react_on_rails/utils.rb, and others.- The test cases involving Shakapacker in
spec/react_on_rails/utils_spec.rbcover different contexts, such as when Shakapacker is enabled or disabled, and when files are present or missing in the manifest.Given this comprehensive coverage, the changes appear to be correctly implemented.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of utility functions with Shakapacker references in different setups or with different inputs. # Test: Setup different inputs and check if the utility functions behave as expected. # This is a conceptual verification, actual commands would depend on access to test scripts or input details. echo "Setup different inputs and verify the behavior of utility functions with Shakapacker references."Length of output: 202
Script:
#!/bin/bash # Find all utility function implementations in the codebase ast-grep --lang ruby --pattern 'def $_($$$) $$$ end' # Find all test cases involving Shakapacker in the spec/react_on_rails/utils_spec.rb file rg --context 5 'Shakapacker' spec/react_on_rails/utils_spec.rbLength of output: 102171
Your changes look promising. However, there are some CI failures to resolve.
Yes, please do. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
spec/react_on_rails/utils_spec.rb (2)
23-35: Ensure consistency in context descriptions and tag usage.The context description on line 23 uses "with Shakapacker enabled", which is correct. However, the tag on line 40 still uses
:webpacker. It should be updated to:shakapackerto maintain consistency and avoid confusion.
[!TIP]
Codebase VerificationThe extensive use of
receive_message_chainin thespec/react_on_rails/utils_spec.rbfile suggests that the underlying objects have complex interactions and deep method chains. This can lead to tightly coupled code and brittle tests. Consider refactoring to simplify these interactions and improve test maintainability.
- Instances of
receive_message_chainfound inspec/react_on_rails/utils_spec.rb:
allow(ReactOnRails).to receive_message_chain(:configuration, :generated_assets_dir)allow(Shakapacker).to receive_message_chain("dev_server.running?")allow(Shakapacker).to receive_message_chain("config.public_output_path")allow(Shakapacker).to receive_message_chain("manifest.lookup!")allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file")allow(ReactOnRails).to receive_message_chain("configuration.node_modules_location")allow(Shakapacker).to receive_message_chain("config.send").with(:data)allow(ReactOnRails).to receive_message_chain("configuration.same_bundle_for_client_and_server")allow(Shakapacker).to receive_message_chain("dev_server.protocol")allow(Shakapacker).to receive_message_chain("dev_server.host_with_port")Analysis chain
Line range hint
102-173: Review the use ofreceive_message_chainfor potential refactoring.The extensive use of
receive_message_chainin this block (lines 102-173) could be a sign of overly coupled code or an indication that the underlying objects are too complex. Consider refactoring or simplifying the interaction with these objects.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the use of `receive_message_chain` for potential refactoring opportunities. rg --type ruby "receive_message_chain" spec/Length of output: 4645
lib/react_on_rails/helper.rb (1)
319-319: Ensure that the method name reflects its functionality.The method
raise_nested_entries_disabledon line 319 might benefit from a more descriptive name that clearly indicates its purpose and the conditions under which it raises an exception.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
spec/dummy/yarn.lockis excluded by!**/*.lock
Files selected for processing (19)
- CHANGELOG.md (2 hunks)
- lib/generators/react_on_rails/install_generator.rb (2 hunks)
- lib/react_on_rails.rb (1 hunks)
- lib/react_on_rails/configuration.rb (6 hunks)
- lib/react_on_rails/helper.rb (2 hunks)
- lib/react_on_rails/packs_generator.rb (3 hunks)
- lib/react_on_rails/shakapacker_utils.rb (5 hunks)
- lib/react_on_rails/test_helper.rb (2 hunks)
- lib/react_on_rails/test_helper/webpack_assets_status_checker.rb (1 hunks)
- lib/react_on_rails/utils.rb (3 hunks)
- spec/dummy/package.json (2 hunks)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1 hunks)
- spec/dummy/spec/packs_generator_spec.rb (13 hunks)
- spec/react_on_rails/configuration_spec.rb (7 hunks)
- spec/react_on_rails/locales_to_js_spec.rb (1 hunks)
- spec/react_on_rails/shakapacker_utils_spec.rb (1 hunks)
- spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb (1 hunks)
- spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb (4 hunks)
- spec/react_on_rails/utils_spec.rb (6 hunks)
Files not reviewed due to errors (4)
- lib/react_on_rails/packs_generator.rb (no review received)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (no review received)
- lib/react_on_rails/configuration.rb (no review received)
- spec/dummy/spec/packs_generator_spec.rb (no review received)
Files skipped from review due to trivial changes (1)
- spec/dummy/package.json
Files skipped from review as they are similar to previous changes (3)
- lib/react_on_rails/utils.rb
- spec/react_on_rails/configuration_spec.rb
- spec/react_on_rails/test_helper/webpack_assets_status_checker_spec.rb
Additional comments not posted (14)
lib/react_on_rails.rb (1)
21-21: Change fromwebpacker_utilstoshakapacker_utilsaligns with the PR's objective to transition from Webpacker to Shakapacker.spec/react_on_rails/test_helper/ensure_assets_compiled_spec.rb (1)
12-12: Replacement ofWebpackerUtilswithShakapackerUtilsis consistent with the PR's objective.spec/react_on_rails/locales_to_js_spec.rb (1)
13-13: Replacement ofWebpackerUtilswithShakapackerUtilsis consistent with the PR's objective.lib/react_on_rails/test_helper/webpack_assets_status_checker.rb (1)
33-34: Replacement ofWebpackerUtilswithShakapackerUtilsis consistent with the PR's objective.lib/react_on_rails/test_helper.rb (1)
Line range hint
70-89: Integration with Shakapacker inensure_assets_compiledmethod looks correct and aligns with the PR's objectives.lib/react_on_rails/shakapacker_utils.rb (5)
4-8: Methodusing_shakapacker?correctly checks for Shakapacker availability.
38-53: Methodbundle_js_uri_from_shakapackercorrectly constructs URIs for JS bundles using Shakapacker.
87-92: Methodcheck_manifest_not_cachedcorrectly handles the scenario where the manifest should not be cached during tests.
122-124: Methodraise_shakapacker_version_incompatible_for_autobundlingcorrectly checks for version compatibility and raises a clear error if incompatible.
133-133: Methodraise_shakapacker_not_installedcorrectly checks if Shakapacker is installed and raises an informative error if not.spec/react_on_rails/utils_spec.rb (2)
31-31: Verify the chaining of method calls for correctness.The use of
receive_message_chainon lines 31 and 33 is generally discouraged as it can lead to brittle tests. It's better to set up each method call individually if possible. This approach makes the tests more resilient to changes in the underlying method chains.Also applies to: 33-34
62-66: Ensure correct method usage and context consistency.The context "without Shakapacker enabled" on line 62 seems to contradict the method call on line 66, which checks if Shakapacker is being used. This could lead to confusion or incorrect test behavior.
lib/react_on_rails/helper.rb (1)
334-334: Verify the path construction for correctness.The path construction on line 334 should be verified to ensure it correctly points to the intended JavaScript file. This is crucial for the correct functioning of the autoload feature.
Verification successful
The
source_entry_pathis configured inshakapacker.ymlaspacks. This means the constructed path inlib/react_on_rails/helper.rbwill bepacks/generated/#{react_component_name}.js, which appears to be correct.
lib/react_on_rails/helper.rbline 334:"#{ReactOnRails::ShakapackerUtils.shakapacker_source_entry_path}/generated/#{react_component_name}.js"This path construction correctly points to the intended JavaScript file within the
packs/generateddirectory.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the path construction for JavaScript files in the Shakapacker integration. rg --type ruby "shakapacker_source_entry_path" lib/Length of output: 668
Script:
#!/bin/bash # Extract the implementation of the shakapacker_source_entry_path method ast-grep --lang ruby --pattern 'def self.shakapacker_source_entry_path $$$ end' lib/react_on_rails/shakapacker_utils.rbLength of output: 346
Script:
#!/bin/bash # Search for the configuration of Shakapacker.config.source_entry_path rg --type ruby "Shakapacker.config.source_entry_path"Length of output: 136
Script:
#!/bin/bash # Search for the definition or assignment of Shakapacker.config.source_entry_path rg --type ruby "source_entry_path"Length of output: 1514
Script:
#!/bin/bash # Search for Shakapacker configuration settings in the codebase rg --type ruby "Shakapacker.config"Length of output: 762
Script:
#!/bin/bash # Search for Shakapacker configuration settings in the codebase rg --type yaml "source_entry_path"Length of output: 195
CHANGELOG.md (1)
25-27: Optimize the modulo operation.- while x >= y: - x -= y - return x + return x % yLikely invalid or redundant comment.
|
I greatly appreciate your work on that. 👍
Correct. Any time we change supported dependency version ranges, then the next release of ReactOnRails will be a new major version since it is a breaking change. |
|
@Judahmeek any update on this? I think the workflow got stuck (might be due to the Github actions incident last week - https://www.githubstatus.com/incidents/nj47vccwm2zj). Thanks! |
|
@justin808 I've tested this branch, and it did work fine. |
|
Thanks for this - I gave it a try locally. The file After running This is because Webpack dropped the |
thanks @timkrins - will make the change @justin808 should we copy-paste the config from https://github.com/shakacode/shakapacker/blob/main/lib/install/config/shakapacker.yml? |
Yes, as it's the current one and the current version is the default. |
CHANGELOG.md
Outdated
| ### [14.0.2] - 2024-05-20 | ||
|
|
||
| #### Removed | ||
| - Dropped Shakapacker v6 support [PR 1622](https://github.com/shakacode/react_on_rails/pull/1622) by [adriangohjw](https://github.com/adriangohjw) |
There was a problem hiding this comment.
This would be a breaking change and require going to v15.
could we still support shakapacker v6?
There was a problem hiding this comment.
@justin808 yes, creating a POC PR for that
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .github/workflows/lint-js-and-ruby.yml (3 hunks)
- .github/workflows/main.yml (7 hunks)
- .github/workflows/package-js-tests.yml (2 hunks)
- .github/workflows/rspec-package-specs.yml (3 hunks)
- lib/react_on_rails/packer_utils.rb (7 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/lint-js-and-ruby.yml
Additional comments not posted (17)
.github/workflows/package-js-tests.yml (2)
14-14: Approved matrix version strategy change.Using descriptive labels ('oldest', 'newest') instead of specific versions enhances flexibility and maintainability of the testing strategy.
23-23: Approved dynamic Node.js version setup.Setting the Node.js version based on the matrix labels ('oldest', 'newest') is a good practice for ensuring the workflow remains up-to-date with supported versions.
.github/workflows/rspec-package-specs.yml (3)
10-10: Job renaming enhances clarity.Renaming the job from 'build' to 'rspec-package-tests' clarifies its purpose, improving the readability of the workflow file.
22-22: Dynamic Ruby version setup approved.Dynamically setting the Ruby version based on 'oldest' and 'newest' labels is a good practice for testing across multiple versions.
37-37: Updated caching strategy is more precise.The new cache key structure, which includes the matrix versions and gemspec dependencies, enhances the accuracy and efficiency of caching.
lib/react_on_rails/packer_utils.rb (7)
4-7: Methodusing_packer?correctly determines the packer type.This method efficiently checks and returns whether Shakapacker or Webpacker is in use, aiding in the dynamic configuration of the application.
19-22: Methodusing_webpacker?correctly handles version checks.This method efficiently determines whether Webpacker or an older version of Shakapacker is in use, which is crucial for backward compatibility.
24-29: Methodpacker_typeis well-implemented.This method accurately determines and returns the type of packer in use, or
nilif neither is available, supporting flexible configuration.
31-39: Methodpackercorrectly handles packer module loading.This method efficiently determines and loads the appropriate packer module (
ShakapackerorWebpacker), ensuring the application uses the correct resources.
43-45: Methoddev_server_running?effectively checks server status.This method correctly checks if the development server of the current packer is running, which is essential for development and testing environments.
49-52: Methodshakapacker_versionis efficiently implemented.This method retrieves and memoizes the Shakapacker version, ensuring efficient access to this information when needed.
61-61: Version requirement check method is robust.The method
shakapacker_version_requirement_met?correctly compares the required version with the installed version, ensuring compatibility..github/workflows/main.yml (5)
13-13: Approved matrix version strategy change.Using descriptive labels ('oldest', 'newest') instead of specific versions enhances flexibility and maintainability of the testing strategy.
22-22: Dynamic Ruby version setup approved.Dynamically setting the Ruby version based on 'oldest' and 'newest' labels is a good practice for testing across multiple versions.
31-31: Approved dynamic Node.js version setup.Setting the Node.js version based on the matrix labels ('oldest', 'newest') is a good practice for ensuring the workflow remains up-to-date with supported versions.
57-57: Updated node_modules caching strategy is more precise.The new cache key structure, which includes the matrix versions, enhances the accuracy and efficiency of caching for node_modules.
67-67: Updated Ruby gems caching strategy is more precise.The new cache key structure, which includes the matrix versions and gemspec dependencies, enhances the accuracy and efficiency of caching for Ruby gems.
lib/react_on_rails/packer_utils.rb
Outdated
| def self.using_shakapacker? | ||
| return @using_shakapacker if defined?(@using_shakapacker) | ||
|
|
||
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | ||
| shakapacker_version_requirement_met?([7, 0, 0]) | ||
| end |
There was a problem hiding this comment.
Consider making Shakapacker version requirement configurable.
The method using_shakapacker? effectively checks for Shakapacker's availability and version. However, the version requirement is hardcoded. Consider making this configurable to enhance flexibility.
- shakapacker_version_requirement_met?([7, 0, 0])
+ shakapacker_version_requirement_met?(SHAKAPACKER_MIN_VERSION)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.
| def self.using_shakapacker? | |
| return @using_shakapacker if defined?(@using_shakapacker) | |
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | |
| shakapacker_version_requirement_met?([7, 0, 0]) | |
| end | |
| def self.using_shakapacker? | |
| return @using_shakapacker if defined?(@using_shakapacker) | |
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | |
| shakapacker_version_requirement_met?(SHAKAPACKER_MIN_VERSION) | |
| end |
.github/workflows/main.yml
Outdated
| key: v5-dummy-app-gem-cache-${{ hashFiles('spec/dummy/Gemfile.lock') }} | ||
| key: dummy-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||
| - name: Dynamically add the right shakapacker version to the Gemfile | ||
| run: echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> spec/dummy/Gemfile |
There was a problem hiding this comment.
Ensure dynamic Gemfile updates are idempotent.
Appending directly to the Gemfile in a CI script can lead to non-idempotent behavior if the script runs multiple times. Consider checking if the gem entry already exists before appending.
- echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> Gemfile
+ if ! grep -q "gem 'shakapacker'" Gemfile; then
+ echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" >> Gemfile
+ fiCommittable 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.
| run: echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> spec/dummy/Gemfile | |
| run: | | |
| if ! grep -q "gem 'shakapacker'" spec/dummy/Gemfile; then | |
| echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" >> spec/dummy/Gemfile | |
| fi |
There was a problem hiding this comment.
Let's have the latest version back in spec/dummy and have the CI script modify the gemfile to test the older version.
There was a problem hiding this comment.
@justin808, I will adjust the CI script to ensure that the spec/dummy Gemfile defaults to the latest version of Shakapacker. The CI script will then conditionally modify the Gemfile to test the older version when necessary. Here's the updated script section:
- name: Set default Shakapacker version in Gemfile
run: |
if ! grep -q "gem 'shakapacker'" spec/dummy/Gemfile; then
echo "gem 'shakapacker', '~> 8.0.0'" >> spec/dummy/Gemfile
fi
- name: Conditionally use older Shakapacker version for testing
if: matrix.versions == 'oldest'
run: |
sed -i "s/gem 'shakapacker', '~> 8.0.0'/gem 'shakapacker', '~> 6.6.0'/" spec/dummy/GemfileThis setup ensures that the Gemfile initially sets the latest version of Shakapacker (8.0.0). If the CI is running with the 'oldest' matrix version, it will replace this with the older version (6.6.0). This approach maintains idempotency and aligns with the testing requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .github/workflows/examples.yml (4 hunks)
- rakelib/example_type.rb (4 hunks)
- rakelib/run_rspec.rake (2 hunks)
- rakelib/shakapacker_examples.rake (1 hunks)
- rakelib/webpacker_examples.rake (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/examples.yml
- rakelib/run_rspec.rake
- rakelib/shakapacker_examples.rake
- rakelib/webpacker_examples.rake
Additional comments not posted (4)
rakelib/example_type.rb (4)
13-13: Reevaluate thread safety of class variable initialization.The change to initialize
@allas a hash with specific keys is a good step towards better organization. However, the issue of thread safety in a multi-threaded environment like Rails still stands.
16-16: Document the constructor parameters for enhanced clarity.The introduction of
packer_typein the constructor and attribute readers aligns with the changes made to support multiple packer types. However, documentation on these parameters is still missing, which could hinder maintainability.Also applies to: 18-22
56-56: Review dynamic method definition for potential maintenance issues.Dynamic definition of methods within a loop can lead to maintenance challenges, especially when debugging or refactoring. Consider documenting or simplifying this pattern.
61-61: New method addition approved.The addition of the
rspec_task_name_shortmethod aligns well with the new naming conventions and supports the PR's objectives effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- rakelib/run_rspec.rake (3 hunks)
- rakelib/shakapacker_examples.rake (1 hunks)
- rakelib/webpacker_examples.rake (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- rakelib/run_rspec.rake
- rakelib/shakapacker_examples.rake
- rakelib/webpacker_examples.rake
.github/workflows/main.yml
Outdated
| key: v5-dummy-app-gem-cache-${{ hashFiles('spec/dummy/Gemfile.lock') }} | ||
| key: dummy-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | ||
| - name: Dynamically add the right shakapacker version to the Gemfile | ||
| run: echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> spec/dummy/Gemfile |
There was a problem hiding this comment.
Let's have the latest version back in spec/dummy and have the CI script modify the gemfile to test the older version.
| bundler: 2.5.9 | ||
| - name: Setup Node | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: ${{ matrix.node }} | ||
| node-version: ${{ matrix.versions == 'oldest' && '16' || '20' }} |
.github/workflows/main.yml
Outdated
| - run: cd spec/dummy && yarn add "shakapacker@${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}" | ||
| - name: Dynamically add the right shakapacker version to Gemfiles | ||
| run: | | ||
| echo "gem 'shakapacker', '~> ${{ matrix.versions == 'oldest' && '6.6.0' || '8.0.0' }}'" >> Gemfile |
There was a problem hiding this comment.
Have https://github.com/shakacode/react_on_rails/blob/master/Gemfile.development_dependencies#L3 use the latest, and use a couple lines of bash code to change to the 6.6.0 when needed.
CONTRIBUTING.md
Outdated
| yalc publish | ||
| cd spec/dummy | ||
| bundle && yarn | ||
| yarn add shakapacker |
There was a problem hiding this comment.
add instructions on how to locally test the older shakapacker
lib/react_on_rails/packer_utils.rb
Outdated
| using_shakapacker? || using_webpacker? | ||
| end | ||
|
|
||
| def self.using_shakapacker? |
|
|
||
| const generateWebpackConfigAlias = generateWebpackConfig ? generateWebpackConfig : () => undefined; | ||
|
|
||
| const baseClientWebpackConfig = v6WebpackConfig ? v6WebpackConfig : generateWebpackConfigAlias(); |
There was a problem hiding this comment.
Revert any complexities here and have CI change the file if needed for tests on 6.6 shakapacker
spec/dummy/config/webpacker.yml
Outdated
| @@ -0,0 +1,73 @@ | |||
| # Note: You must restart bin/shakapacker-dev-server for changes to take effect | |||
| @@ -3,13 +3,15 @@ | |||
| require "rails_helper" | |||
| require "support/script_tag_utils" | |||
|
|
|||
| Packer = Object.const_get(ReactOnRails::PackerUtils.packer_type.capitalize) | |||
There was a problem hiding this comment.
Can we try calling const_get in PackerUtils?
|
|
||
| describe "rake assets:precompile task" do | ||
| it "doesn't show deprecation message for using webpacker:clean task" do | ||
| allow(ENV).to receive(:[]).with(anything).and_call_original | ||
| allow(ENV).to receive(:[]).with("SHAKAPACKER_PRECOMPILE").and_return("false") | ||
| allow(ENV).to receive(:[]).with("WEBPACKER_PRECOMPILE").and_return("false") |
There was a problem hiding this comment.
Yes, when using option 1 to stub environment variables in RSpec tests, it's a good practice to revert the original value after the test is completed. This ensures that the stubbed value doesn't affect other tests or the overall test environment.
You can use RSpec's before and after hooks to set up and clean up the stubbed environment variable. Here's an example:
describe "My test" do
before do
@original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = @original_value
end
it "tests something" do
# Your test code here
end
endIn this example:
-
The
beforehook is used to store the original value of the environment variable in an instance variable (@original_value) before stubbing it. This allows you to restore the original value later. -
The
allowmethod is used to stub theENVobject to return the desired stubbed value when accessed with the specific variable name. -
The
afterhook is used to clean up the stubbed environment variable. It first resets the stubbed behavior by callingallow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original, which tells RSpec to revert to the original behavior ofENVfor that variable. -
Finally, the original value stored in
@original_valueis assigned back to the environment variable usingENV["VARIABLE_NAME"] = @original_value.
By following this approach, you ensure that the stubbed environment variable is properly cleaned up and reverted to its original value after each test, preventing any unintended side effects on other tests.
There was a problem hiding this comment.
Overall, add a rake task to modify the files of spec/dummy to test Shakapacker 6.6.0. Check before running the task that there are no uncommitted files and error if there are.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
spec/dummy/config/webpack/webpack.config.js (1)
Line range hint
11-13: Simplify the conditional logic.As the static analysis suggests, the
elseclause is unnecessary because the previous branch (theifstatement) covers all possibilities that would prevent reaching theelseclause. This simplification enhances readability and maintainability.if (existsSync(path)) { console.log(`Loading ENV specific webpack configuration file ${path}`); return require(path); - } else { - return generateWebpackConfig(); - } + } + return generateWebpackConfig();
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (14)
- .github/workflows/examples.yml (5 hunks)
- .github/workflows/lint-js-and-ruby.yml (3 hunks)
- .github/workflows/main.yml (10 hunks)
- .github/workflows/package-js-tests.yml (2 hunks)
- .github/workflows/rspec-package-specs.yml (3 hunks)
- Gemfile.development_dependencies (1 hunks)
- rakelib/example_type.rb (4 hunks)
- rakelib/run_rspec.rake (3 hunks)
- rakelib/shakapacker_examples.rake (1 hunks)
- rakelib/webpacker_examples.rake (3 hunks)
- script/convert (1 hunks)
- spec/dummy/config/webpack/commonWebpackConfig.js (1 hunks)
- spec/dummy/config/webpack/webpack.config.js (1 hunks)
- spec/dummy/package.json (2 hunks)
Files skipped from review due to trivial changes (2)
- Gemfile.development_dependencies
- spec/dummy/package.json
Files skipped from review as they are similar to previous changes (8)
- .github/workflows/examples.yml
- .github/workflows/lint-js-and-ruby.yml
- .github/workflows/main.yml
- .github/workflows/package-js-tests.yml
- .github/workflows/rspec-package-specs.yml
- rakelib/shakapacker_examples.rake
- rakelib/webpacker_examples.rake
- spec/dummy/config/webpack/commonWebpackConfig.js
Additional context used
Biome
spec/dummy/config/webpack/webpack.config.js
[error] 11-13: This else clause can be omitted because previous branches break early.
Unsafe fix: Omit the else clause.
(lint/style/noUselessElse)
Additional comments not posted (1)
rakelib/example_type.rb (1)
13-13: Ensure correct initialization and storage ofExampleTypeinstances.The modifications to the
ExampleTypeclass include changes to how instances are stored and initialized. Ensure that thepacker_typeparameter is correctly handled and that instances are stored appropriately in a class variable.Also applies to: 16-22
Verification successful
Initialization and storage of
ExampleTypeinstances are correctly handled.Both the initialization and storage logic for
packer_typeare present and correctly implemented in therakelib/example_type.rbfile.
- Initialization:
@packer_type = packer_type- Storage:
self.class.all[packer_type.to_sym] << selfScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that ExampleType instances are correctly initialized and stored. # Check if `packer_type` is correctly handled grep -q "@packer_type = packer_type" "rakelib/example_type.rb" && echo "Initialization logic is correct." || echo "Initialization error: packer_type not handled correctly." # Check storage of instances grep -q "self.class.all[packer_type.to_sym] << self" "rakelib/example_type.rb" && echo "Storage logic is correct." || echo "Storage error: instances not stored correctly."Length of output: 319
Script:
#!/bin/bash # Searching for storage logic in the rakelib/example_type.rb file rg "self.class.all" rakelib/example_type.rbLength of output: 94
| 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", "8.0.0"', 'gem "shakapacker", "6.6.0"') | ||
|
|
||
| gsub_file_content("../spec/dummy/package.json", '"shakapacker": "8.0.0",', '"shakapacker": "6.6.0",') | ||
|
|
||
| gsub_file_content("../spec/dummy/config/webpack/commonWebpackConfig.js", /generateWebpackConfig(\(\))?/, | ||
| "webpackConfig") | ||
|
|
||
| gsub_file_content("../spec/dummy/config/webpack/webpack.config.js", /generateWebpackConfig(\(\))?/, "webpackConfig") |
There was a problem hiding this comment.
Tip
Codebase Verification
Review file paths and replacement logic in conversion script.
The paths specified in the script do not exist, and the content replacement logic could not be verified because the target files are missing. Please review and update the paths and ensure the files exist in the expected locations.
-
Paths not found:
../spec/dummy/config/shakapacker.yml../spec/dummy/config/webpacker.yml../Gemfile.development_dependencies../spec/dummy/package.json../spec/dummy/config/webpack/commonWebpackConfig.js../spec/dummy/config/webpack/webpack.config.js
-
Content replacement logic could not be verified due to missing files.
Analysis chain
Review file paths and replacement logic in conversion script.
The script performs critical operations such as renaming and content replacement. Ensure that the paths provided are correct relative to the script's execution context, and verify that the replacement logic correctly reflects the intended changes from shakapacker to webpacker.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the paths and content replacement logic in the conversion script.
# Check if paths exist
if [ -f "../spec/dummy/config/shakapacker.yml" ] && [ -f "../spec/dummy/config/webpacker.yml" ]; then
echo "Paths are correct."
else
echo "Check the paths. They might be incorrect."
fi
# Check content replacement
grep -q 'gem "shakapacker", "8.0.0"' "../Gemfile.development_dependencies" && echo "Replacement logic is correct." || echo "Check the replacement logic."
Length of output: 353
Script:
#!/bin/bash
# Verify the paths and content replacement logic in the conversion script.
# Check if paths exist
paths=(
"../spec/dummy/config/shakapacker.yml"
"../spec/dummy/config/webpacker.yml"
"../Gemfile.development_dependencies"
"../spec/dummy/package.json"
"../spec/dummy/config/webpack/commonWebpackConfig.js"
"../spec/dummy/config/webpack/webpack.config.js"
)
for path in "${paths[@]}"; do
if [ -f "$path" ]; then
echo "Path exists: $path"
else
echo "Path does not exist: $path"
fi
done
# Check content replacement logic
grep -q 'gem "shakapacker", "8.0.0"' "../Gemfile.development_dependencies" && echo "Found string in Gemfile.development_dependencies" || echo "String not found in Gemfile.development_dependencies"
grep -q '"shakapacker": "8.0.0",' "../spec/dummy/package.json" && echo "Found string in package.json" || echo "String not found in package.json"
grep -q 'generateWebpackConfig(' "../spec/dummy/config/webpack/commonWebpackConfig.js" && echo "Found string in commonWebpackConfig.js" || echo "String not found in commonWebpackConfig.js"
grep -q 'generateWebpackConfig(' "../spec/dummy/config/webpack/webpack.config.js" && echo "Found string in webpack.config.js" || echo "String not found in webpack.config.js"
Length of output: 2480
| @@ -66,12 +53,12 @@ def rails_options | |||
| method_name_normal = "#{task_type}_task_name" # ex: `clean_task_name` | |||
| method_name_short = "#{method_name_normal}_short" # ex: `clean_task_name_short` | |||
|
|
|||
| define_method(method_name_normal) { "#{self.class.namespace_name}:#{task_type}_#{name}" } | |||
| define_method(method_name_normal) { "#{@packer_type}:#{task_type}_#{name}" } | |||
There was a problem hiding this comment.
Review dynamic method definition for potential maintenance issues.
Dynamic definition of methods within a loop can lead to maintenance challenges, especially when debugging or refactoring. Consider documenting or simplifying this pattern.
# Dynamically defines task name methods for different operations
TASK_TYPES.each do |task_type|
define_method("#{task_type}_task_name") { "#{@packer_type}:#{task_type}_#{name}" }
define_method("#{task_type}_task_name_short") { "#{task_type}_#{name}" }
end| # Loads data from examples_config.yml and instantiates corresponding ExampleType objects | ||
| examples_config_file = File.expand_path("examples_config.yml", __dir__) | ||
| examples_config = symbolize_keys(YAML.safe_load_file(examples_config_file)) | ||
| examples_config[:example_type_data].each do |example_type_data| | ||
| ExampleType.new(packer_type: "shakapacker_examples", **symbolize_keys(example_type_data)) | ||
| ExampleType.new(packer_type: "webpacker_examples", **symbolize_keys(example_type_data)) | ||
| end |
There was a problem hiding this comment.
Tip
Codebase Verification
Conditional logic error: not handling different scenarios correctly.
The conditional logic for invoking Rake tasks with different packer values (i.e., shakapacker and webpacker) is not correctly implemented. This needs to be addressed to ensure the correct handling of different scenarios in the rakelib/run_rspec.rake file.
- Lines 13-19: Ensure the conditional logic correctly handles different
packervalues. - Also applies to: 41-47, 49-51, 58-65, 81-82
Analysis chain
Review dynamic Rake task definitions and execution logic.
The modifications to this file include the dynamic definition of Rake tasks based on example types and the inclusion of conditional logic for task execution. Ensure that the tasks are correctly defined and that the conditional logic is robust and correctly handles different scenarios.
Also applies to: 41-47, 49-51, 58-65, 81-82
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the dynamic Rake task definitions and execution logic.
# Check task definitions
grep -q "task example_type.rspec_task_name_short => example_type.gen_task_name" "rakelib/run_rspec.rake" && echo "Task definitions are correct." || echo "Task definition error: tasks not defined correctly."
# Check conditional logic
grep -q "Rake::Task[\"run_rspec:#{packer}_examples\"].invoke" "rakelib/run_rspec.rake" && echo "Conditional logic is correct." || echo "Conditional logic error: not handling different scenarios correctly."
Length of output: 405
Script:
#!/bin/bash
# Verify the dynamic Rake task definitions and execution logic with refined search for conditional logic.
# Check task definitions
grep -q "task example_type.rspec_task_name_short => example_type.gen_task_name" "rakelib/run_rspec.rake" && echo "Task definitions are correct." || echo "Task definition error: tasks not defined correctly."
# Check conditional logic for different packer values
grep -q 'Rake::Task\["run_rspec:shakapacker_examples"\].invoke' "rakelib/run_rspec.rake" && echo "Shakapacker conditional logic is correct." || echo "Shakapacker conditional logic error: not handling different scenarios correctly."
grep -q 'Rake::Task\["run_rspec:webpacker_examples"\].invoke' "rakelib/run_rspec.rake" && echo "Webpacker conditional logic is correct." || echo "Webpacker conditional logic error: not handling different scenarios correctly."
Length of output: 687
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/main.yml (10 hunks)
- .github/workflows/rspec-package-specs.yml (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/main.yml
- .github/workflows/rspec-package-specs.yml
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/main.yml (10 hunks)
- .github/workflows/rspec-package-specs.yml (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/main.yml
- .github/workflows/rspec-package-specs.yml
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
CONTRIBUTING.md (1)
Line range hint
9-25: Clarify the purpose of each step in the testing instructions.The instructions for running tests are detailed but could benefit from additional clarity on what each command accomplishes.
+ # This command prepares all examples and runs all tests: bundle && yarn && rake examples:gen_all && rake node_package && rake + # To run tests in a browser, use the following commands:Tools
Markdownlint
138-138: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CONTRIBUTING.md (1 hunks)
- lib/react_on_rails/packer_utils.rb (7 hunks)
Additional context used
LanguageTool
CONTRIBUTING.md
[style] ~15-~15: Consider a shorter alternative to avoid wordiness.
Context: ...n_all && rake node_package && rakeIn order to run tests in browseryarn global ad...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~25-~25: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...E/editor to ignore certain directories. Otherwise your IDE might slow to a crawl! * /cov...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[style] ~69-~69: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...led files yarn run build-watchYou need to do this once:# Will send the upda...(REP_NEED_TO_VB)
[style] ~79-~79: Consider shortening or rephrasing this to strengthen your wording.
Context: ...react-on-rails ``` The workflow is: 1. Make changes to the node package. 2. We need yalc to pu...(MAKE_CHANGES)
[style] ~178-~178: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...figs, then you need to restart foreman. If you change the JS code for react-on-rai...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~196-~196: Possible missing comma found.
Context: ...s. ### Install Generator In your Rails app add this gem with a path to your fork. ...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~226-~226: The word ‘lint’ is a noun or an adjective. A verb or adverb is missing or misspelled here, or maybe a comma is missing.
Context: ...changes via the lint container. You can lint locally by running `npm run lint && npm...(PRP_MD_NN)
[grammar] ~231-~231: The word ‘install’ is not a noun.
Context: ...he initial build is slow, but after the install, startup is very quick. ### Linting Co...(A_INSTALL)
[style] ~231-~231: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...slow, but after the install, startup is very quick. ### Linting Commands Runrake lint....(EN_WEAK_ADJECTIVE)
[typographical] ~238-~238: Consider adding a comma here.
Context: ...rs. For individual rake linting commands please refer torake -D dockerfor the list....(PLEASE_COMMA)
[grammar] ~261-~261: If this is a question, the word order is probably incorrect. Did you mean “Why is this”… ?
Context: ...ub.com/turbolinks/turbolinks) repo: > [Why this is not still fully merged?](https://github...(WHY_THE_WORD_ORDER_IS_WRONG)
Markdownlint
CONTRIBUTING.md
101-101: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time(MD001, heading-increment)
200-200: Expected: 0 or 2; Actual: 61
Trailing spaces(MD009, no-trailing-spaces)
138-138: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
7-7: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
24-24: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
39-39: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
49-49: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
58-58: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
101-101: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
109-109: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
177-177: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
180-180: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
195-195: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
220-220: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
225-225: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
233-233: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
244-244: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
250-250: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
24-24: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
39-39: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
105-105: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
257-257: null
Multiple top-level headings in the same document(MD025, single-title, single-h1)
7-7: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
17-17: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
41-41: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
81-81: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
8-8: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
79-79: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
80-80: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
102-102: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
102-102: null
Bare URL used(MD034, no-bare-urls)
17-17: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
41-41: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
71-71: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
81-81: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
93-93: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
210-210: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
216-216: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Additional comments not posted (20)
lib/react_on_rails/packer_utils.rb (20)
4-7: LGTM: Correct implementation ofusing_packer?method.This method effectively determines if either Shakapacker or Webpacker is being used, which is essential for conditional logic throughout the module.
16-22: LGTM: Effective use of conditional logic inusing_webpacker_const?.This method correctly handles the scenario where either Webpacker or an older version of Shakapacker might be used, ensuring backward compatibility.
24-29: LGTM: Correctly determines the packer type.The method
packer_typeeffectively returns the packer type based on the conditions provided, which is crucial for other methods that depend on this information.
31-39: LGTM: Correct implementation ofpackermethod.This method dynamically requires and returns the appropriate packer module, which is essential for integrating with the rest of the application.
43-45: LGTM: Correctly checks if the development server is running.This method ensures that a packer is in use before checking the development server status, which prevents errors in environments where no packer is configured.
49-52: LGTM: Efficient retrieval of Shakapacker version.The method uses memoization to avoid unnecessary recomputation and checks for Shakapacker availability, which is a robust approach to handling dependencies.
61-61: Assumption of correct implementation forshakapacker_version_as_array.This method is used in version checks and seems to function correctly based on its usage in other parts of the code.
61-61: LGTM: Robust version comparison inshakapacker_version_requirement_met?.This method correctly uses
Gem::Versionfor comparing versions, ensuring accurate and reliable checks.
87-91: LGTM: Correct conditional handling inprecompile?.This method effectively determines the precompilation configuration based on the packer type, which is crucial for build processes.
94-96: LGTM: Direct and clear retrieval ofpacker_source_path.This method provides a straightforward way to retrieve the source path configuration from the packer, which is essential for asset management.
98-99: LGTM: Direct and clear retrieval ofpacker_source_entry_path.This method provides a straightforward way to retrieve the source entry path configuration from the packer, which is essential for asset management.
103-103: LGTM: Correct check for nested entries configuration.This method directly checks the packer configuration to determine if nested entries are enabled, which is important for asset compilation.
111-111: LGTM: Correct check for manifest file existence.This method effectively checks if the manifest file exists in the public path, which is crucial for ensuring that assets are properly managed and served.
114-115: LGTM: Correct check for explicitly set source path.This method checks if the source path is explicitly set in the packer configuration, which is important for asset management and avoids potential misconfigurations.
119-124: LGTM: Correct handling of manifest caching in test environments.This method correctly checks if the manifest is cached and provides a clear error message if it is, which is essential for avoiding issues during testing.
Line range hint
137-137: Assumption of correct implementation forwebpack_assets_status_checker.This method is used to initialize a status checker for webpack assets, and it seems to function correctly based on its usage in other parts of the code.
145-145: LGTM: Correct error handling for disabled nested entries.This method raises a detailed error if nested entries are disabled in the Shakapacker configuration, ensuring that developers are aware of necessary configuration changes.
154-156: LGTM: Correct error handling for incompatible Shakapacker version.This method raises a detailed error if the Shakapacker version is incompatible for autobundling, ensuring that developers are aware of version requirements.
165-165: LGTM: Correct error handling for missing Shakapacker installation.This method raises a detailed error if Shakapacker is not installed, ensuring that developers are aware of the necessary dependencies.
173-173: LGTM: Correct implementation ofsemver_to_string.This utility method effectively converts a semantic version array to a string, which is commonly used throughout the module for version handling.
| def self.bundle_js_uri_from_packer(bundle_name) | ||
| hashed_bundle_name = packer.manifest.lookup!(bundle_name) | ||
|
|
||
| # Support for hashing the server-bundle and having that built | ||
| # the webpack-dev-server is provided by the config value | ||
| # "same_bundle_for_client_and_server" where a value of true | ||
| # would mean that the bundle is created by the webpack-dev-server | ||
| is_server_bundle = bundle_name == ReactOnRails.configuration.server_bundle_js_file | ||
|
|
||
| if Webpacker.dev_server.running? && (!is_server_bundle || | ||
| if packer.dev_server.running? && (!is_server_bundle || | ||
| ReactOnRails.configuration.same_bundle_for_client_and_server) | ||
| "#{Webpacker.dev_server.protocol}://#{Webpacker.dev_server.host_with_port}#{hashed_bundle_name}" | ||
| "#{packer.dev_server.protocol}://#{packer.dev_server.host_with_port}#{hashed_bundle_name}" |
There was a problem hiding this comment.
Consider adding unit tests for bundle_js_uri_from_packer.
This method handles complex scenarios for generating URIs based on the development server status and configuration. It correctly handles different cases, but given its complexity, adding unit tests would ensure its reliability.
Would you like assistance in writing these unit tests?
lib/react_on_rails/packer_utils.rb
Outdated
| def self.using_shakapacker_const? | ||
| return @using_shakapacker if defined?(@using_shakapacker) | ||
|
|
||
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | ||
| shakapacker_version_requirement_met?([7, 0, 0]) | ||
| end |
There was a problem hiding this comment.
Consider making the Shakapacker version requirement configurable.
The version requirement is hardcoded, which could limit flexibility if version requirements change in the future.
- shakapacker_version_requirement_met?([7, 0, 0])
+ shakapacker_version_requirement_met?(SHAKAPACKER_MIN_VERSION)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.
| def self.using_shakapacker_const? | |
| return @using_shakapacker if defined?(@using_shakapacker) | |
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | |
| shakapacker_version_requirement_met?([7, 0, 0]) | |
| end | |
| def self.using_shakapacker_const? | |
| return @using_shakapacker if defined?(@using_shakapacker) | |
| @using_shakapacker = ReactOnRails::Utils.gem_available?("shakapacker") && | |
| shakapacker_version_requirement_met?(SHAKAPACKER_MIN_VERSION) | |
| end |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/react_on_rails/packer_utils.rb (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/packer_utils.rb
Summary
closes #1621 by replacing
Webpacker.config.shakapacker_precompile?in configuration toShakapacker.config.shakapacker_precompile?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
~.Update documentationOther Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
shakapackerversions in workflows.shakapacker.Refactor
WebpackerUtilsmodule toPackerUtilswith updated methods and logic.Chores
Documentation