docs: Fix CSS Modules exportLocalsConvention documentation#626
Conversation
Corrects the v9 upgrade documentation to specify 'camelCaseOnly' instead of 'camelCase' for exportLocalsConvention when namedExport: true is enabled. Changes: - Update v9_upgrade.md to use 'camelCaseOnly' in configuration examples - Add warning about css-loader compatibility requirements - Clarify difference between 'camelCase' and 'camelCaseOnly' - Add troubleshooting section for build errors - Update css-modules-export-mode.md with detailed explanation - Add error message examples and solutions This addresses the issue where users following the docs would encounter: "exportLocalsConvention" with "camelCase" value is incompatible with "namedExport: true" option Fixes #622 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughDocumentation updates clarify CSS Modules behavior in v9: default exportLocalsConvention is changed to 'camelCaseOnly', constraints when namedExport is true are documented, error scenarios and troubleshooting are added, and examples/migration guidance are expanded in css-modules and v9 upgrade docs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
PR Review: CSS Modules exportLocalsConvention Documentation FixSummaryThis PR correctly fixes a critical documentation error where the v9 upgrade guide incorrectly specified ✅ Strengths
🔍 Issues & Concerns1. Critical: TODO_v9.md Not UpdatedThe file exportLocalsConvention: 'camelCase' // ❌ Should be 'camelCaseOnly'This file should be updated for consistency, or clarified if it's documenting historical/planned behavior. File location: 2. Potential Inconsistency: CHANGELOG.mdThe CHANGELOG.md:96 states: This appears to be the same documentation error. Should this also be corrected to File location: 3. Actual Code Configuration UnclearThe actual webpack configuration in modules: {
auto: true
}Questions:
4. Missing Test CoverageThere don't appear to be any automated tests validating:
While 📝 Suggestions
🔒 SecurityNo security concerns identified. This is purely a documentation fix. ⚡ PerformanceNo performance impact - documentation only. 📊 Test Coverage Assessment
Final RecommendationApprove with requested changes - The documentation fixes are valuable and correct, but the following should be addressed: Must Fix:
Nice to Have: Great work identifying and fixing this documentation bug! This will save users from frustrating build errors. 👍 |
Code Review: Documentation Fix for CSS Modules exportLocalsConventionSummaryThis PR addresses issue #622 by correcting the documentation around the ✅ Strengths1. Comprehensive Coverage
2. User-Focused Improvements
3. Well-Structured
📋 Code Quality AssessmentDocumentation Standards: ✅ Excellent
Accuracy: ✅ Verified
🔍 Detailed Observationsdocs/v9_upgrade.md
docs/css-modules-export-mode.md
|
This commit addresses critical inconsistencies between documentation and implementation for CSS modules configuration in v9. Changes: 1. **Implemented actual CSS modules configuration** (package/utils/getStyleRule.ts) - Added `namedExport: true` to css-loader options - Added `exportLocalsConvention: 'camelCaseOnly'` to css-loader options - Previously only had `auto: true`, so the v9 defaults were not actually applied - Added explanatory comments about css-loader compatibility requirements 2. **Fixed documentation inconsistencies** - Updated TODO_v9.md: Changed 'camelCase' to 'camelCaseOnly' - Updated CHANGELOG.md: Changed 'camelCase' to 'camelCaseOnly' - Updated tools/README.md: Changed 'camelCase' to 'camelCaseOnly' - Added note explaining css-loader compatibility requirements 3. **Updated test expectations** (spec/shakapacker/css_modules_spec.rb) - Changed expected value from 'camelCase' to 'camelCaseOnly' - All tests passing Key Issue Resolved: - Documentation claimed v9 uses `exportLocalsConvention: 'camelCase'` - css-loader actually requires 'camelCaseOnly' or 'dashesOnly' when namedExport: true - Using 'camelCase' with namedExport: true causes build error - Implementation was missing both namedExport and exportLocalsConvention settings This ensures the code matches the documentation and prevents the build error described in issue #622. Related to #622 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: CSS Modules exportLocalsConvention Documentation FixSummaryThis PR successfully addresses issue #622 by correcting the documentation to specify ✅ Strengths1. Accurate Technical Content
2. Comprehensive Coverage
3. User-Friendly Approach
4. Consistent Formatting
|
Code Review - PR #626: Fix CSS Modules exportLocalsConvention DocumentationSummaryThis PR correctly fixes a critical documentation bug where ✅ Code Quality & Best PracticesStrengths:
Code Implementation ( modules: {
auto: true,
// v9 defaults: Use named exports with camelCase conversion
// Note: css-loader requires 'camelCaseOnly' or 'dashesOnly' when namedExport is true
// Using 'camelCase' with namedExport: true causes a build error
namedExport: true,
exportLocalsConvention: 'camelCaseOnly'
}✅ Correct implementation with clear documentation ✅ Bug Prevention & AccuracyCritical Fix:
Proper Error Handling:
✅ Documentation QualityExcellent additions:
Minor suggestion for improvement: // Alternative if you want to keep kebab-case in JS:
exportLocalsConvention: 'dashesOnly' // exports 'my-button', not 'myButton'✅ Test CoverageTest Updates ( configs = [
{ namedExport: false, description: "v8 mode" },
{ namedExport: true, exportLocalsConvention: "camelCaseOnly", description: "v9 mode" }
]✅ Test correctly updated to validate Recommendation: ✅ Performance ConsiderationsNo performance impact. This is purely a configuration and documentation fix. ✅ Security ConcernsNo security issues identified. The changes are limited to:
✅ Breaking Changes & MigrationWell handled:
📋 Pre-merge ChecklistBefore merging, please verify:
🎯 Overall AssessmentRating: Excellent ✅ This is a high-quality PR that:
Recommendation: Approve and merge after running the linting/testing checklist above. 💡 Minor Enhancements (Optional)
Great work! This will help many users avoid the build error when upgrading. 🎉 |
Code Review - PR #626OverviewThis PR fixes a critical documentation issue for CSS Modules in v9, addressing issue #622. Critical Issues Found1. Missing Trailing Newlines (BLOCKER)
2. Implementation Verification Needed 3. Test Inconsistency Strengths
Pre-Merge Requirements
Assessment |
Enhances CSS modules documentation to include the 'dashesOnly' option as an alternative to 'camelCaseOnly' for users who prefer to keep kebab-case class names in their JavaScript code. Changes: - Added dashesOnly option explanation to v9_upgrade.md - Added dashesOnly usage examples with import syntax - Enhanced css-modules-export-mode.md with detailed comparison - Clarified when to use each exportLocalsConvention option - Added practical examples showing both camelCaseOnly and dashesOnly usage Users can now choose between: - 'camelCaseOnly' (default): Converts my-button to myButton - 'dashesOnly': Preserves my-button as-is This gives users more flexibility based on their naming preferences. Related to #622 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Adds comprehensive tests to ensure the CSS modules configuration stays correct and prevents regressions. Changes: - Added test validating 'camelCase' is incompatible with namedExport: true - Added test documenting valid configuration alternatives - Added test ensuring getStyleRule.ts uses valid configuration - Tests verify implementation matches documentation The new tests: 1. Document the exact error case (namedExport: true + camelCase) 2. Validate all three valid configuration combinations 3. Verify the actual source code has the correct configuration 4. Check for explanatory comments in the code This prevents regressions where someone might accidentally change the configuration back to the invalid 'camelCase' value. All tests passing (16 examples, 0 failures, 5 pending) Related to #622 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Fix CSS Modules exportLocalsConvention DocumentationSummaryThis PR correctly fixes a critical documentation bug where ✅ Strengths1. Comprehensive Documentation Coverage
2. Code Implementation (package/utils/getStyleRule.ts:35-42)
3. Test Coverage (spec/shakapacker/css_modules_spec.rb)
4. User Experience Improvements
📝 Code Quality ObservationsExcellent Practices:
🔍 Minor Suggestions1. Test File Consistency (spec/shakapacker/css_modules_spec.rb:145)The new test at line 145 documents behavior but doesn't actually execute validation: it "validates that 'camelCase' is incompatible with namedExport: true" do
# This test documents the exact error that would occur with incorrect configuration
# css-loader will reject this configuration with an errorSuggestion: Consider adding a skip message or renaming to clarify this is documentation: it "documents that 'camelCase' is incompatible with namedExport: true" do
# This test documents the css-loader compatibility requirementThis makes it clearer that it's a documentation test rather than an integration test. 2. Documentation Formatting (Minor)In docs/css-modules-export-mode.md:283-331, the Option A and Option B structure is excellent. Consider using the same structure consistently in v9_upgrade.md for parallel consistency. 🔒 Security Considerations✅ No security concerns - documentation-only changes with proper code implementation ⚡ Performance Considerations✅ No performance impact - configuration change doesn't affect runtime performance 🧪 Test Coverage Assessment
📋 Checklist CompliancePer CLAUDE.md project guidelines:
🎯 Overall AssessmentVerdict: Approved with minor suggestions ✅ This is an excellent documentation fix that:
The changes are accurate, well-explained, and will significantly improve the upgrade experience for v9 users. Recommended Pre-Merge Actions# Verify linting passes
bundle exec rubocop
yarn lint
# Run full test suite
bundle exec rspec
# Verify the actual error can be reproduced (optional validation)
# This would require a test project setupGreat work on this comprehensive documentation fix! 🎉 |
Enhances the doctor command to detect and warn about CSS modules
configuration issues, helping developers catch the incompatible
configuration before build failures occur.
New checks added:
1. Invalid configuration detection
- Detects namedExport: true with exportLocalsConvention: 'camelCase'
- Reports as critical issue with clear fix instructions
- References docs/v9_upgrade.md for detailed guidance
2. Missing configuration detection
- Warns when CSS module files exist without explicit configuration
- Informs about v9 defaults (namedExport: true, camelCaseOnly)
3. V8-style import pattern detection
- Scans JavaScript/TypeScript files for v8 default import patterns
- Warns about needed migration to v9 named exports
- Provides migration examples in warning message
Implementation details:
- Checks webpack config files (webpack.config.js, commonWebpackConfig.js)
- Scans first 50 JS/TS files for import patterns (performance optimization)
- Graceful error handling - doesn't fail doctor on check errors
- Only runs when CSS module files (*.module.css/scss/sass) exist
Test coverage:
- Tests for invalid configuration detection
- Tests for valid configurations (camelCaseOnly, dashesOnly)
- Tests for missing configuration
- Tests for v8/v9 import pattern detection
- All 73 tests passing
Example doctor output:
❌ CSS Modules: Invalid configuration detected in config/webpack/webpack.config.js
Using exportLocalsConvention: 'camelCase' with namedExport: true will cause build errors
Change to 'camelCaseOnly' or 'dashesOnly'. See docs/v9_upgrade.md for details
Related to #622
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: CSS Modules exportLocalsConvention Documentation FixOverall Assessment✅ Excellent PR - This is a critical documentation fix that addresses a real pain point for users upgrading to v9. The changes are comprehensive, well-documented, and include proper testing and validation. Strengths1. Comprehensive Documentation Coverage
2. Correct Technical Implementation
3. Excellent Testing
4. Proactive Error Detection
Suggestions & Observations1. Doctor.rb Implementation (lib/shakapacker/doctor.rb:444-491)The new Performance consideration: css_module_files = Dir.glob(File.join(source_path, "**/*.module.{css,scss,sass}"))This could be slow on large projects. Consider adding a limit or early return if you find just one file: # Just check if ANY CSS module files exist
css_module_exists = Dir.glob(File.join(source_path, "**/*.module.{css,scss,sass}"), File::FNM_CASEFOLD).first
return unless css_module_existsRegex safety: config_content.match(/namedExport\s*:\s*true/)This handles most cases well. ✅ 2. Documentation Clarity (docs/css-modules-export-mode.md:35-72)The "Important: exportLocalsConvention with namedExport" section is excellent. It clearly shows:
This is exactly what users need when encountering this error! 3. Test Coverage (spec/shakapacker/css_modules_spec.rb:144-177)Love the meta-test that validates the actual source code: it "ensures getStyleRule.ts uses valid configuration" do
style_rule_content = File.read("package/utils/getStyleRule.ts")
expect(style_rule_content).to include("exportLocalsConvention: 'camelCaseOnly'")
endThis is a great pattern for ensuring docs and code stay in sync! ✅ 4. Minor: File EndingsAccording to CLAUDE.md, all files must end with a trailing newline. Please verify all modified files have this (particularly the TypeScript and Ruby files). Security & Performance✅ No security concerns - Documentation and configuration changes only Code Quality✅ Follows project conventions - Consistent with existing Shakapacker patterns Test Coverage AssessmentExcellent coverage across:
Suggestion: Consider adding an integration test that actually runs webpack with the invalid config to verify the error message, but this may be overkill given the comprehensive unit tests. Potential Issues FoundNone - This is a well-crafted PR. The only minor items:
Recommendation✅ APPROVE with minor pre-merge checklist:
This PR excellently addresses issue #622 and will save users significant frustration during the v8→v9 upgrade. The comprehensive documentation, proactive error detection, and thorough testing make this a model PR. Great work! 🎉 |
Code Review - PR #626: Fix CSS Modules exportLocalsConvention DocumentationOverall AssessmentThis is an excellent PR that addresses a critical documentation issue (#622) and goes beyond by adding proactive tooling to prevent users from encountering the error. The changes are well-structured, thoroughly tested, and significantly improve the developer experience. Code Quality & Best PracticesStrengths:
The implementation is clean and follows Ruby best practices with defensive programming, early returns for performance, clear separation of concerns, and limited file scanning (first 50 files) to avoid performance issues. Potential Issues & Suggestions1. Error Handling - Minor Risk Issue: Broad exception catching with rescue suppresses all errors. While this prevents doctor from crashing, it might hide unexpected bugs. Recommendation: Be more specific about expected exceptions to differentiate between file access errors and unexpected bugs. 2. Import Pattern Detection - False Positives Issue: The regex might match commented-out code or strings, leading to false warnings. Example: Comment like "Old approach: import styles from styles.module.css" would trigger the warning Recommendation: Add a check to skip comments before pattern matching. 3. Test Documentation Gap The test "validates that camelCase is incompatible with namedExport: true" does not actually validate the error - it just documents valid/invalid configurations. Consider renaming to better reflect its purpose. Security ConsiderationsStatus: No security concerns identified
Performance ConsiderationsStrengths:
Potential Optimization: For very large files, consider adding file size limits to skip files over 1MB. Test CoverageExcellent coverage:
Documentation QualityOutstanding improvements:
Recommendations SummaryPriority: High
Priority: Medium
Priority: Low
ConclusionThis is a high-quality PR that effectively solves the reported issue and adds valuable tooling to prevent future occurrences. The combination of documentation fixes, proactive error detection, and comprehensive testing makes this a significant improvement to the v9 migration experience. Recommendation: APPROVE with minor suggestions The suggested improvements are optional enhancements that do not block merging. The current implementation is solid and will significantly help users avoid the configuration error. Great work! |
Performance optimization: - Changed CSS module file detection from globbing all files to early exit - Uses Dir.glob().first instead of collecting all matches - Significantly faster on large projects with many CSS module files File formatting fixes: - Added trailing newlines to all modified files per CLAUDE.md requirement: - package/utils/getStyleRule.ts - docs/css-modules-export-mode.md - TODO_v9.md - tools/README.md All tests passing (89 examples, 0 failures) All linting passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review - PR #626OverviewThis PR fixes critical documentation errors regarding CSS Modules configuration in v9, specifically correcting exportLocalsConvention from camelCase to camelCaseOnly when namedExport true is enabled. This is an important fix that addresses issue #622. ✅ Strengths1. Thorough Documentation Updates
2. Code Implementation
3. Enhanced Doctor Diagnostics
4. Test Coverage
🔍 Issues & SuggestionsCritical IssuesNone - The code is solid and well-implemented. Minor Issues & Improvements1. File Endings (REQUIRED by CLAUDE.md)Several files are missing trailing newlines, which will fail linting:
Action Required: Add trailing newlines to all files before merge. 2. Doctor Performance ConsiderationIn lib/shakapacker/doctor.rb:492-508, the import pattern check reads up to 50 JavaScript files. Consider using lazy evaluation to avoid loading all files into memory at once. 3. Regex Pattern RobustnessIn lib/shakapacker/doctor.rb:452, the regex patterns could potentially match inside comments or strings, though this is probably fine for most real-world cases. 4. Documentation EnhancementThe migration guide could benefit from a quick reference table showing the differences between configuration options at a glance. 🛡️ Security ConcernsNone identified - This is a documentation and configuration fix with no security implications. ⚡ Performance Considerations
🧪 Test CoverageExcellent Coverage ✅
📋 Best Practices Compliance✅ Follows Project Guidelines (CLAUDE.md)
🎯 Recommended Actions Before MergeMust Fix
Should Consider
📊 Overall AssessmentRecommendation: Approve with minor fixes ✅ This is an excellent PR that:
The only blocking issue is the missing trailing newlines, which is a project requirement. Once those are added and linting passes, this is ready to merge. Impact: High value - prevents user build failures and provides clear migration path. Code Quality: Excellent - well-tested, well-documented, and thoughtfully implemented. Great work! 🎉 |
Performance improvements: - Replaced eager file loading with lazy evaluation in import pattern check - Uses .lazy.take(50) to avoid loading all file paths into memory - Extracts regex pattern to variable for better readability and reuse - Stops immediately after finding first v8-style import Documentation enhancements: - Added comprehensive quick reference tables to v9_upgrade.md - Added configuration comparison table to css-modules-export-mode.md - Tables clearly show valid vs invalid configuration combinations - Visual indicators (✅/❌) for quick scanning - Shows practical examples of what each configuration exports Tables include: - namedExport values - exportLocalsConvention options - Export behavior for kebab-case class names - Use cases for each configuration - Compatibility status All tests passing (89 examples, 0 failures) All linting passing All files have required trailing newlines 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request ReviewScore: 9/10 - Excellent PR ✅ APPROVED SummaryThis PR fixes a critical documentation issue where v9 upgrade docs incorrectly specified 'camelCase' instead of 'camelCaseOnly' for exportLocalsConvention with namedExport: true. Great comprehensive fix! Strengths
Minor Issues (Consider for Future)1. Regex Precision (lib/shakapacker/doctor.rb:470) 2. Silent Error Handling (lib/shakapacker/doctor.rb:511-513) 3. Performance Optimization (lib/shakapacker/doctor.rb:498) 4. Multiple Config Files (lib/shakapacker/doctor.rb:464-481) Security & Performance✅ No security concerns - file reads are scoped, no injection risks Test CoverageExcellent! Covers invalid configs, valid configs, v8/v9 imports, and implementation validation. Suggested additions:
Final VerdictReady to merge! Minor suggestions are optimizations, not blockers. This addresses a critical user pain point with excellent documentation and proactive detection. Great work! 🎉 Reviewed by Claude Code following CLAUDE.md guidelines |
1 participant
Summary
Fixes the v9 upgrade documentation to correctly specify
exportLocalsConvention: 'camelCaseOnly'instead of'camelCase'whennamedExport: trueis enabled.Problem
The documentation incorrectly suggested using
exportLocalsConvention: 'camelCase', but css-loader requires either'camelCaseOnly'or'dashesOnly'whennamedExport: trueis enabled. This caused build errors for users following the upgrade guide.Changes
docs/v9_upgrade.md
'camelCaseOnly'instead of'camelCase''camelCase'and'camelCaseOnly'docs/css-modules-export-mode.md
'camelCase'(both versions) and'camelCaseOnly'(only camelCase)'camelCaseOnly'Impact
Users upgrading to v9 will now have correct configuration examples and clear guidance when they encounter the compatibility error.
Fixes #622
🤖 Generated with Claude Code
Summary by CodeRabbit