Export bundler utility functions for Webpack/Rspack compatibility #880
Conversation
|
Warning Rate limit exceeded
⌛ 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. ✨ 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 |
Greptile OverviewGreptile SummaryIntroduced bundler utility functions that simplify writing configuration code compatible with both Webpack and Rspack, eliminating the need for manual conditional logic. Key Changes
CHANGELOG NoteAccording to CLAUDE.md guidelines, this PR adds user-visible features (new utility exports) and should have a CHANGELOG entry. However, no CHANGELOG.md changes were included in this PR. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Code
participant Index as package/index.ts
participant BundlerUtils as bundlerUtils.ts
participant Config as config
participant RequireOrError as requireOrError
participant Webpack as webpack
participant Rspack as @rspack/core
User->>Index: require('shakapacker')
Index->>BundlerUtils: import bundler utilities
User->>BundlerUtils: getBundler()
BundlerUtils->>Config: read assets_bundler
Config-->>BundlerUtils: 'webpack' or 'rspack'
alt isWebpack
BundlerUtils->>RequireOrError: require('webpack')
RequireOrError->>Webpack: load module
Webpack-->>RequireOrError: webpack module
RequireOrError-->>BundlerUtils: webpack module
BundlerUtils-->>User: webpack module
else isRspack
BundlerUtils->>RequireOrError: require('@rspack/core')
RequireOrError->>Rspack: load module
Rspack-->>RequireOrError: @rspack/core module
RequireOrError-->>BundlerUtils: @rspack/core module
BundlerUtils-->>User: @rspack/core module
end
User->>BundlerUtils: getCssExtractPlugin()
BundlerUtils->>Config: read assets_bundler
alt isWebpack
BundlerUtils->>RequireOrError: require('mini-css-extract-plugin')
RequireOrError-->>BundlerUtils: MiniCssExtractPlugin
BundlerUtils-->>User: MiniCssExtractPlugin
else isRspack
BundlerUtils->>RequireOrError: require('@rspack/core')
RequireOrError-->>BundlerUtils: @rspack/core
BundlerUtils->>BundlerUtils: extract CssExtractRspackPlugin
BundlerUtils-->>User: CssExtractRspackPlugin
end
User->>BundlerUtils: getDefinePlugin()
BundlerUtils->>BundlerUtils: call getBundler()
BundlerUtils->>BundlerUtils: extract DefinePlugin
BundlerUtils-->>User: DefinePlugin (webpack or rspack)
|
Code Review for PR #880Great work on this PR! The bundler utility functions are a nice addition that will improve the developer experience for users switching between Webpack and Rspack. Here's my detailed review: ✅ Strengths
🔍 Code Quality ObservationsTypeScript Type SafetyThe use of
Consideration: While the current approach is consistent with the project, you might want to add a TypeScript declaration file (e.g., Test CoverageThe tests are thorough but focus on the default (Webpack) case. Consider adding:
Example test to add: describe("when bundler is rspack", () => {
beforeAll(() => {
// Mock config to return 'rspack'
jest.mock("../../package/config", () => ({ assets_bundler: "rspack" }))
})
test("isRspack is true and isWebpack is false", () => {
expect(index.isRspack).toBe(true)
expect(index.isWebpack).toBe(false)
})
})Potential Issues1. Missing Trailing Newline ✓Checked all files - they properly end with newlines. Good! 2. Export ConsistencyBoth
This is intentional based on the existing pattern, but worth noting for consistency. 🎯 Suggestions (Optional Improvements)1. Consider Memoization for PerformanceThe getter functions call let _bundlerCache: unknown | null = null
const getBundler = (): unknown => {
if (\!_bundlerCache) {
_bundlerCache = isRspack ? requireOrError("@rspack/core") : requireOrError("webpack")
}
return _bundlerCache
}This is a micro-optimization and not critical since 2. Add Runtime Type GuardsFor better safety, you could add runtime checks: const getDefinePlugin = (): unknown => {
const bundler = getBundler() as { DefinePlugin: unknown }
if (\!bundler.DefinePlugin) {
throw new Error('[SHAKAPACKER]: DefinePlugin not found in bundler module')
}
return bundler.DefinePlugin
}This would catch unexpected API changes in webpack/rspack. 3. Update CHANGELOG.mdPer the project's CLAUDE.md guidelines, this user-visible feature should be documented in CHANGELOG.md. You can use the 🔒 Security & Performance
📋 Final Checklist
VerdictApprove with minor suggestions. This is solid work that will make Shakapacker easier to use. The suggestions above are enhancements, not blockers. The only required change per project guidelines is updating CHANGELOG.md. Great job! 🚀 |
Code Review - PR #880: Export bundler utility functionsSummaryThis PR adds excellent bundler-agnostic utility functions that simplify the developer experience when working with both Webpack and Rspack. The implementation is clean, well-documented, and thoroughly tested. ✅ Strengths
🔍 Code Quality Observationspackage/utils/bundlerUtils.ts:33-44The const isWebpack: boolean = config.assets_bundler !== "rspack"This works because package/utils/bundlerUtils.ts:84-90
const getCssExtractPlugin = (): unknown => {
if (isRspack) {
const rspack = requireOrError("@rspack/core") // First call
return rspack.CssExtractRspackPlugin
}
return requireOrError("mini-css-extract-plugin")
}The same pattern appears in package/utils/bundlerUtils.ts:139-186The plugin getter functions ( 🧪 Test Coveragetest/package/bundlerUtils.test.jsThe tests are well-structured:
Potential Enhancement: Consider adding tests for the rspack mode by temporarily modifying the config, though this may require test infrastructure changes. Current tests adequately cover the default webpack scenario. ⚡ Performance Considerations
🔒 Security ConsiderationsNo security concerns identified:
📋 Suggestions (Optional Enhancements)
✅ Final VerdictRecommendation: Approve and Merge This is a high-quality PR that:
The code is production-ready. The minor optimizations mentioned above are truly optional and the current implementation prioritizes code clarity and maintainability, which is the right choice. Great work! 🎉 |
Code Review - PR #880SummaryThis PR adds bundler utility functions to simplify writing bundler-agnostic configuration. The implementation is clean, well-documented, and follows the project's patterns. ✅ Strengths
💡 Suggestions1. TypeScript Return Type SpecificityThe functions currently return const getBundler = (): unknown =>
isRspack ? requireOrError("@rspack/core") : requireOrError("webpack")Consider: While I understand this is intentional (added to ESLint's type safety improvement list), for better DX you could add type overloads or conditional types in a future PR: type BundlerModule = typeof import('webpack') | typeof import('@rspack/core')
const getBundler = (): BundlerModule => ...This would enable autocompletion for users. However, this is not a blocker and can be addressed in the type safety improvement pass. 2. Memoization OpportunityThe let cachedBundler: unknown
const getBundler = (): unknown => {
if (\!cachedBundler) {
cachedBundler = isRspack ? requireOrError("@rspack/core") : requireOrError("webpack")
}
return cachedBundler
}Note: This is a micro-optimization and not necessary for correctness. The current implementation is fine. 3. Test Enhancement - Rspack Code PathThe tests only verify the webpack code path (default behavior). Consider adding tests for the rspack path in a follow-up or documenting that rspack-specific testing requires a different test environment setup. // Could add in a separate test suite with mocked config
describe('when bundler is rspack', () => {
beforeEach(() => {
// Mock config.assets_bundler = 'rspack'
})
// Test rspack-specific return values
})However, this might require test infrastructure changes, so it's okay to defer. 🔒 SecurityNo security concerns identified. The code uses existing ⚡ PerformanceNo performance concerns. The utilities are simple getter functions with minimal overhead. 🧪 TestingTest coverage is good for the default webpack case. All tests pass according to the PR description. 📋 Nitpicks
✅ Project Guidelines Compliance
🎯 RecommendationAPPROVE - This is a well-implemented feature that provides clear value to users. The code quality is high, documentation is excellent, and tests are comprehensive. The minor suggestions above are optional enhancements that don't block merging. Great work! This will make it much easier for users to write bundler-agnostic configurations. 🚀 |
Code Review - PR #880: Export bundler utility functionsSummaryThis PR adds excellent bundler-agnostic utility functions that significantly improve the developer experience when working with both Webpack and Rspack. The implementation is clean, well-documented, and thoroughly tested. ✅ Strengths1. Code Quality & Design
2. Error Handling
3. Test Coverage
4. Documentation
🔍 Minor Observations1. Performance ConsiderationIn const getCssExtractPlugin = (): unknown => {
if (isRspack) {
const rspack = requireOrError("@rspack/core") // First load
return rspack.CssExtractRspackPlugin
}
return requireOrError("mini-css-extract-plugin")
}
const getCssExtractPluginLoader = (): string => {
if (isRspack) {
const rspack = requireOrError("@rspack/core") // Second load
return rspack.CssExtractRspackPlugin.loader
}
// ...
}Impact: Node.js module caching makes this a non-issue - subsequent 2. Type Safety Trade-offThe use of Why this is acceptable:
3. ESLint ConfigurationThe file is added to the list requiring relaxed type safety rules (eslint.config.js:236). This is reasonable given the dynamic 🔒 SecurityNo concerns identified
🎯 Best Practices Alignment✅ Follows CLAUDE.md guidelines:
✅ Code conventions:
📝 Suggestions (Optional)Consider caching for repeated callsIf users call these functions repeatedly in their config (unlikely but possible), consider memoization: let cachedBundler: unknown
const getBundler = (): unknown => {
if (!cachedBundler) {
cachedBundler = isRspack ? requireOrError("@rspack/core") : requireOrError("webpack")
}
return cachedBundler
}Note: This is likely premature optimization since webpack configs are loaded once. Current implementation is cleaner. Future enhancement: Type guardsConsider adding type guards for advanced users: export const isWebpackBundler = (bundler: unknown): bundler is typeof webpack => isWebpackNote: Low priority - the boolean flags are sufficient for most use cases. ✨ Overall AssessmentRecommendation: ✅ APPROVE This is a high-quality PR that:
The implementation is production-ready. Great work! 🎉 Test Results: Per PR description, all 361 tests pass, ESLint passes, and TypeScript compiles without errors. |
Code Review SummaryExcellent work on this PR! The bundler utility functions are a well-designed addition that significantly improves the developer experience. Here's my detailed review: ✅ StrengthsCode Quality
API Design
Testing
Documentation
💡 Suggestions for Improvement1. Test Coverage for Rspack ModeThe tests currently only verify webpack mode (the default). Consider adding tests that verify rspack mode behavior: describe('when bundler is rspack', () => {
// Would need to mock config.assets_bundler = 'rspack'
test('isRspack returns true', () => {
// Test rspack-specific behavior
})
})Note: This may require refactoring to allow dependency injection or config mocking. Not a blocker, but would increase confidence. 2. Type Safety in bundlerUtils.tsThe file is added to the ESLint ignore list for type safety issues. While pragmatic for now, consider:
Recommendation: File a technical debt issue to improve type safety in a future PR. 3. Documentation EnhancementConsider adding these utility functions to the documentation:
This would help users discover these helpful functions. 4. Minor: Plugin Getter ConsistencyFunctions like let cachedBundler: BundlerModule | null = null
const getBundler = (): BundlerModule => {
if (!cachedBundler) {
cachedBundler = (isRspack ? requireOrError('@rspack/core') : requireOrError('webpack')) as BundlerModule
}
return cachedBundler
}Note: Only optimize if needed - current approach is simpler and clearer. 🔒 Security & Performance
📋 Checklist
Verdict✅ Approve with minor suggestions This PR is ready to merge. The suggestions above are nice-to-haves that could be addressed in follow-up PRs. The core implementation is solid, well-tested, and provides clear value to users. The bundler-agnostic API is a significant UX improvement that makes Shakapacker configurations cleaner and easier to maintain. Great work! 🎉 Files Reviewed
🤖 Review generated with Claude Code |
Add bundler utility functions that make it easier to write bundler-agnostic
configuration code. Users no longer need to write conditional logic to handle
differences between Webpack and Rspack.
New exports from shakapacker:
- isRspack: Boolean indicating if current bundler is Rspack
- isWebpack: Boolean indicating if current bundler is Webpack
- getBundler(): Returns the bundler module (webpack or @rspack/core)
- getCssExtractPlugin(): Returns MiniCssExtractPlugin or CssExtractRspackPlugin
- getCssExtractPluginLoader(): Returns the CSS extraction loader string
- getDefinePlugin(): Returns the DefinePlugin for the current bundler
- getEnvironmentPlugin(): Returns the EnvironmentPlugin for the current bundler
- getProvidePlugin(): Returns the ProvidePlugin for the current bundler
Usage example:
const { getCssExtractPlugin, isRspack } = require('shakapacker')
const CssPlugin = getCssExtractPlugin()
new CssPlugin({ filename: 'styles.css' })
Closes #875
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Improves developer experience by providing proper TypeScript types: - BundlerModule: Interface describing the bundler module - PluginConstructor: Common interface for plugin constructors - CssExtractPluginConstructor: Typed CSS extraction plugin - CssExtractPluginOptions: Options for CSS extraction plugins Users now get autocompletion when using these utilities. Co-Authored-By: Claude Opus 4.5 <[email protected]>
1 participant
Summary
New Exports
isRspackisWebpackgetBundler()getCssExtractPlugin()getCssExtractPluginLoader()getDefinePlugin()getEnvironmentPlugin()getProvidePlugin()Usage Examples
Before (manual conditional logic):
After (using new utilities):
Test Plan
Closes #875
🤖 Generated with Claude Code