Migrate to Shakapacker 9.0.0-beta.8 #665
Conversation
WalkthroughShakapacker upgraded from 9.0.0.beta.7 → 9.0.0.beta.8 in Gemfile and package.json. Webpack SCSS rule handling in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / Process
participant WP as commonWebpackConfig.js
participant Rules as Rules Array
participant Inject as Loader Injection
Dev->>WP: require/load config
WP->>Rules: find rule where test matches /\.scss/
alt SCSS rule with use[] found
Rules-->>WP: return scssRule with use[]
WP->>Rules: locate sass-loader & css-loader indices (optional chaining)
alt loader indices valid
WP->>Inject: push/insert sassLoaderConfig into scssRule.use
Inject-->>WP: updated rule
else missing loaders
WP-->>Dev: skip injection (guarded)
end
else no SCSS rule or no use[]
WP-->>Dev: no-op / warning (guarded)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
PR Review: Migrate to Shakapacker 9.0.0-beta.8SummaryThis PR updates Shakapacker from beta.7 to beta.8 and fixes webpack configuration to handle the new SCSS rule structure. Overall, this is a well-executed dependency update with appropriate defensive coding. Code Quality ✅Strengths:
Suggestions:
Potential Bugs 🔍Minor Issue:
Performance Considerations ⚡No performance concerns. The changes are configuration-time operations that run once during webpack setup. Security Concerns 🔒No security issues identified. This is a dependency update with configuration adjustments. Test Coverage 📋Observations:
Recommendation:
Additional Comments
VerdictApprove with minor suggestions. The core fix is correct and addresses the breaking change appropriately. The suggested improvements are minor and focus on edge case handling and code clarity. 🤖 Generated by Claude Code |
![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
🧹 Nitpick comments (1)
config/webpack/commonWebpackConfig.js (1)
39-52: Minor: Redundant undefined check.The
sassLoaderIndex !== undefinedcheck is technically redundant sinceArray.prototype.findIndexonly returns-1(when not found) or a valid index (0 or positive integer), neverundefined. However, this defensive check is harmless and may improve code clarity.If you prefer to remove the redundant check:
-if (sassLoaderIndex !== -1 && sassLoaderIndex !== undefined) { +if (sassLoaderIndex !== -1) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
Gemfile(1 hunks)config/webpack/commonWebpackConfig.js(3 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 ESLint
config/webpack/commonWebpackConfig.js
[error] 56-56: Replace loader with (loader)
(prettier/prettier)
⏰ 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). (1)
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (5)
Gemfile (1)
9-9: LGTM!Version bump to Shakapacker 9.0.0.beta.8 aligns with the PR objectives and matches the corresponding package.json update.
package.json (1)
96-96: LGTM!Shakapacker dependency updated to 9.0.0-beta.8, consistent with the Gemfile change.
config/webpack/commonWebpackConfig.js (3)
27-28: LGTM!The additional
config.usecheck correctly filters for SCSS rules that actually contain loaders, avoiding the new oneOf pattern at index 0 in beta.8 that matches.scssbut lacks ausearray.
32-37: LGTM!Optional chaining safely navigates the
scssRule.usearray, preventing errors when the SCSS rule lacks loaders.
65-67: LGTM!The guard correctly prevents attempting to push to a non-existent
usearray, aligning with the defensive null-safety approach throughout this change.
|
/deploy-review-app |
|
✅ Deployment complete for PR #665, commit 2d00493 |
Updates Shakapacker from beta.7 to beta.8. The main change required was fixing the webpack config to properly detect SCSS rules, as the rule structure changed in beta.8 to include a new oneOf pattern at index 0. Changes: - Update Shakapacker version in Gemfile and package.json - Fix commonWebpackConfig.js to find SCSS rules that have a 'use' array - Add null safety checks for scssRule properties 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
justin808/shakapacker-9-beta8
branch
from
2d00493 to
ccea269
Compare
|
✅ Deployment complete for PR #665, commit ccea269 |
Pull Request Review: Migrate to Shakapacker 9.0.0-beta.8SummaryThis PR successfully updates Shakapacker from beta.7 to beta.8 and addresses a critical webpack configuration issue introduced by structural changes in the new version. The implementation is solid with good defensive programming practices. ✅ Code Quality & Best PracticesStrengths:
Suggestions:
🐛 Potential Bugs or IssuesNo critical bugs found, but some considerations:
⚡ Performance ConsiderationsNo performance concerns identified:
🔒 Security ConcernsNo security issues found:
Recommendation: Consider reviewing the Shakapacker 9.0.0-beta.8 changelog for any security-related updates or breaking changes beyond the webpack rules structure. 🧪 Test CoverageCurrent state:
Recommendations:
📋 Additional Notes
✨ ConclusionThis is a well-implemented fix for a breaking change in Shakapacker beta.8. The defensive programming approach is appropriate, and the code quality is high. Recommendation: Approve with minor suggestions The suggestions above are optional improvements that would enhance debuggability and test coverage, but they don't block merging this PR. The core fix is sound and addresses the issue correctly. Great work on identifying and fixing the Review conducted with focus on code quality, potential bugs, performance, security, and test coverage as requested. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
Gemfile(1 hunks)config/webpack/commonWebpackConfig.js(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- Gemfile
🧰 Additional context used
🪛 ESLint
config/webpack/commonWebpackConfig.js
[error] 48-48: Insert ,
(prettier/prettier)
[error] 49-49: Insert ,
(prettier/prettier)
⏰ 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). (2)
- GitHub Check: deploy
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (5)
config/webpack/commonWebpackConfig.js (5)
26-28: LGTM: Proper filtering for SCSS rules with loaders.The addition of
&& config.usecorrectly filters out SCSS rules that match the test pattern but lack ausearray, ensuring only rules with actual loaders are selected. This addresses the structural changes in Shakapacker 9.0.0-beta.8.
30-32: LGTM: Defensive guard clause.The early return with a warning message provides clear feedback when no SCSS rule with a
usearray is found, preventing potential runtime errors in the configuration logic below.
34-40: LGTM: Robust sass-loader detection.The logic correctly handles both string and object loader formats with proper null-safety checks, ensuring the sass-loader is accurately identified regardless of its configuration format.
57-66: LGTM: CSS modules configuration.The css-loader lookup and
exportLocalsConventionadjustment properly ensures compatibility whennamedExportis enabled, preventing potential runtime errors with CSS modules.
68-68: LGTM: Safe sass-resources-loader addition.Moving this line inside the guarded else block ensures the
sass-resources-loaderis only added when a valid SCSS rule with ausearray exists, preventing potential runtime errors.
| if (sassLoaderIndex !== -1) { | ||
| const sassLoader = scssRule.use[sassLoaderIndex]; | ||
| if (typeof sassLoader === 'string') { | ||
| scssRule.use[sassLoaderIndex] = { | ||
| loader: sassLoader, | ||
| options: { | ||
| api: 'modern' | ||
| } | ||
| }; | ||
| } else { | ||
| sassLoader.options = sassLoader.options || {}; | ||
| sassLoader.options.api = 'modern'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix ESLint formatting issues.
ESLint/Prettier flagged missing trailing commas on lines 48 and 49.
Apply this diff:
loader: sassLoader,
options: {
- api: 'modern'
+ api: 'modern',
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 48-48: Insert ,
(prettier/prettier)
[error] 49-49: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In config/webpack/commonWebpackConfig.js around lines 42 to 55, ESLint/Prettier
reported missing trailing commas on lines 48 and 49; update the object literals
to include trailing commas (add a trailing comma after the api: 'modern'
property and ensure the enclosing options object/loader object entries end with
trailing commas) so the scssRule.use replacement conforms to the project's
formatting rules.
|
✅ Review app for PR #665 was successfully deleted |
Summary
Updates Shakapacker from 9.0.0-beta.7 to 9.0.0-beta.8.
Changes
Gemfileandpackage.jsoncommonWebpackConfig.jsto properly detect SCSS rules with the new webpack config structure in beta.8Technical Details
In beta.8, the webpack rules structure changed to include a new
oneOfpattern at index 0 that matches.scssbut doesn't have ausearray. The fix ensures we find the SCSS rule that actually has loaders configured.Test Plan
yarn build:test)🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Bug Fixes
Chores