Supersede #913: bundler utility exports with lint unblock#922
Supersede #913: bundler utility exports with lint unblock#922
Conversation
WalkthroughAdds a bundler-agnostic utilities module and exports to expose bundler detection and plugin accessors (isRspack, isWebpack, getBundler, getCssExtractPlugin, getCssExtractPluginLoader, getDefinePlugin, getEnvironmentPlugin, getProvidePlugin); updates types, re-exports from package and rspack entry points, adds tests, and documents the change in CHANGELOG.md. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryThis PR adds bundler utility functions (
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A["User Config Code"] --> B["require('shakapacker')"]
B --> C["package/index.ts"]
B --> D["package/rspack/index.ts"]
C --> E["bundlerUtils.ts"]
D --> E
E --> F{"config.assets_bundler"}
F -->|"rspack"| G["requireOrError('@rspack/core')"]
F -->|"webpack"| H["requireOrError('webpack')"]
G --> I["isRspack = true\nisWebpack = false"]
H --> J["isRspack = false\nisWebpack = true"]
G --> K["CssExtractRspackPlugin"]
H --> L["mini-css-extract-plugin"]
G --> M["rspack.DefinePlugin\nrspack.EnvironmentPlugin\nrspack.ProvidePlugin"]
H --> N["webpack.DefinePlugin\nwebpack.EnvironmentPlugin\nwebpack.ProvidePlugin"]
Last reviewed commit: 2012f6d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const getBundler = (): BundlerModule => | ||
| (isRspack | ||
| ? requireOrError("@rspack/core") | ||
| : requireOrError("webpack")) as BundlerModule |
There was a problem hiding this comment.
Redundant requireOrError calls per plugin getter
Each call to getDefinePlugin(), getEnvironmentPlugin(), or getProvidePlugin() invokes getBundler(), which calls requireOrError() every time. While Node.js caches require() results so there's no real performance issue, consider caching the bundler module if these getters are called frequently in hot paths. This is a minor stylistic point — the current approach is functionally correct.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Acknowledged. Node.js caches require() results so there's no real performance concern here. Leaving as-is for now since it's functionally correct and keeps each getter self-contained.
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - **Export bundler utility functions for Webpack/Rspack compatibility**. [PR #880](https://github.com/shakacode/shakapacker/pull/880) by [justin808](https://github.com/justin808). New utility functions that make it easier to write bundler-agnostic configuration code: `isRspack`, `isWebpack`, `getBundler()`, `getCssExtractPlugin()`, `getCssExtractPluginLoader()`, `getDefinePlugin()`, `getEnvironmentPlugin()`, and `getProvidePlugin()`. Users no longer need to write conditional logic to handle differences between Webpack and Rspack. |
There was a problem hiding this comment.
CHANGELOG references PR #880 instead of #922
The changelog entry links to PR #880, but this PR is #922 (superseding #913). If #880 was the original feature PR that was never merged, this link will point to a closed/unmerged PR. Consider updating to reference this PR (#922) so the link points to the actual merged change.
| - **Export bundler utility functions for Webpack/Rspack compatibility**. [PR #880](https://github.com/shakacode/shakapacker/pull/880) by [justin808](https://github.com/justin808). New utility functions that make it easier to write bundler-agnostic configuration code: `isRspack`, `isWebpack`, `getBundler()`, `getCssExtractPlugin()`, `getCssExtractPluginLoader()`, `getDefinePlugin()`, `getEnvironmentPlugin()`, and `getProvidePlugin()`. Users no longer need to write conditional logic to handle differences between Webpack and Rspack. | |
| - **Export bundler utility functions for Webpack/Rspack compatibility**. [PR #922](https://github.com/shakacode/shakapacker/pull/922) by [justin808](https://github.com/justin808). New utility functions that make it easier to write bundler-agnostic configuration code: `isRspack`, `isWebpack`, `getBundler()`, `getCssExtractPlugin()`, `getCssExtractPluginLoader()`, `getDefinePlugin()`, `getEnvironmentPlugin()`, and `getProvidePlugin()`. Users no longer need to write conditional logic to handle differences between Webpack and Rspack. |
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
justin808
force-pushed
the
codex/supersede-913-bundler-utils-lint
branch
from
2012f6d to
92247d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 16: Update the CHANGELOG entry to reference the actual merging PR:
replace the `[PR `#880`](https://github.com/shakacode/shakapacker/pull/880)`
fragment with `[PR `#922`](https://github.com/shakacode/shakapacker/pull/922)` and
ensure the author attribution remains `by
[justin808](https://github.com/justin808)` so the entry reads: `[PR
`#922`](https://github.com/shakacode/shakapacker/pull/922) by
[justin808](https://github.com/justin808)` while leaving the rest of the text
(function list: `isRspack`, `isWebpack`, `getBundler()`,
`getCssExtractPlugin()`, `getCssExtractPluginLoader()`, `getDefinePlugin()`,
`getEnvironmentPlugin()`, `getProvidePlugin()`) unchanged.
🧹 Nitpick comments (3)
package/utils/bundlerUtils.ts (1)
161-168:getCssExtractPluginLoaderlacks the type assertions used ingetCssExtractPlugin.
getCssExtractPlugin(Line 129) explicitly casts therequireOrErrorreturn value, butgetCssExtractPluginLoaderaccesses.CssExtractRspackPlugin.loaderand.loaderon untyped results without assertions. While this works at runtime (and the ESLint overrides suppress warnings), adding consistent type narrowing would improve readability and make the code more resilient if lint suppressions are ever removed.Suggested fix
const getCssExtractPluginLoader = (): string => { if (isRspack) { - const rspack = requireOrError("@rspack/core") + const rspack = requireOrError("@rspack/core") as { + CssExtractRspackPlugin: CssExtractPluginConstructor + } return rspack.CssExtractRspackPlugin.loader } - const MiniCssExtractPlugin = requireOrError("mini-css-extract-plugin") + const MiniCssExtractPlugin = requireOrError( + "mini-css-extract-plugin" + ) as CssExtractPluginConstructor return MiniCssExtractPlugin.loader }test/package/bundlerUtils.test.js (2)
1-97: Missing test coverage for the Rspack bundler path.All tests verify only the default webpack behavior. The rspack code path (e.g., loading
@rspack/core, returningCssExtractRspackPlugin) is not tested at all. Consider adding a parallel test suite that mocksconfig.assets_bundleras"rspack"(viajest.mock) to validate the rspack branch of each utility.Based on learnings: "Test changes with both webpack and rspack configurations when modifying core functionality."
29-35: Consider verifying plugin identity, not just existence.The
getBundlertest checks thatDefinePlugin,EnvironmentPlugin, andProvidePluginaretoBeDefined(), but doesn't verify they are the expected constructor (e.g.,expect(bundler.DefinePlugin).toBe(require('webpack').DefinePlugin)). This would catch subtle regressions where the wrong module is loaded.
justin808
added
codex
- Update CHANGELOG entry to reference PR #922 instead of #880 - Add bundler utility exports (isRspack, isWebpack, getBundler, getCssExtractPlugin, etc.) to ShakapackerExports interface in index.d.ts and index.d.ts.template so TypeScript consumers get proper type checking Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Bundler Utility ExportsOverall this is clean, well-documented work. The API design is sensible and the JSDoc examples are genuinely helpful. A few things worth addressing before merging: Issues1. getCssExtractPluginLoader duplicates the @rspack/core require from getCssExtractPlugin Both functions load 2. Tests only cover the webpack code path Every test relies on the default webpack bundler. The rspack branches in 3. isWebpack is defined as !== 'rspack' rather than === 'webpack' Any unrecognized 4. CssExtractPluginOptions includes webpack-only options in the shared type
5. Internal duplication opportunity (optional)
Minor
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const getCssExtractPluginLoader = (): string => { | ||
| if (isRspack) { | ||
| const rspack = requireOrError("@rspack/core") | ||
| return rspack.CssExtractRspackPlugin.loader | ||
| } | ||
| const MiniCssExtractPlugin = requireOrError("mini-css-extract-plugin") | ||
| return MiniCssExtractPlugin.loader | ||
| } |
There was a problem hiding this comment.
This duplicates the @rspack/core require already performed inside getCssExtractPlugin. Consider delegating:
| const getCssExtractPluginLoader = (): string => { | |
| if (isRspack) { | |
| const rspack = requireOrError("@rspack/core") | |
| return rspack.CssExtractRspackPlugin.loader | |
| } | |
| const MiniCssExtractPlugin = requireOrError("mini-css-extract-plugin") | |
| return MiniCssExtractPlugin.loader | |
| } | |
| const getCssExtractPluginLoader = (): string => getCssExtractPlugin().loader |
This keeps both functions in sync automatically and avoids loading @rspack/core twice.
There was a problem hiding this comment.
Acknowledged — Node.js caches require() results so the redundant call has no real cost. Leaving as-is for simplicity.
| * // Webpack-specific configuration | ||
| * } | ||
| */ | ||
| const isWebpack: boolean = config.assets_bundler !== "rspack" |
There was a problem hiding this comment.
Using !== "rspack" means any future or mistyped assets_bundler value (e.g. "vite", "webpack5") silently resolves to isWebpack === true, which could cause the wrong plugins to be loaded. Since config.ts already normalises missing values to "webpack", using === "webpack" here would be symmetric with isRspack and safer:
| const isWebpack: boolean = config.assets_bundler !== "rspack" | |
| const isWebpack: boolean = config.assets_bundler === "webpack" |
There was a problem hiding this comment.
Acknowledged. The \!== "rspack" pattern is intentional — webpack is the default/fallback bundler, and this is consistent with how config.assets_bundler is used elsewhere in the codebase (e.g., isRspack checks for === "rspack" throughout). Adding explicit validation for unknown bundler values would be a separate concern.
| attributes?: Record<string, string> | ||
| linkType?: string | false | ||
| runtime?: boolean | ||
| experimentalUseImportModule?: boolean |
There was a problem hiding this comment.
experimentalUseImportModule is a webpack-only mini-css-extract-plugin option — it doesn't exist in Rspack's CssExtractRspackPlugin. Placing it in the shared interface risks silently confusing rspack users who pass it and get no effect. A brief comment noting it is webpack-only, or splitting into separate WebpackCssExtractPluginOptions / RspackCssExtractPluginOptions, would clarify intent.
There was a problem hiding this comment.
Good observation. The interface is intentionally a union of common options that work across both bundlers. The property will simply be ignored by rspack if passed, and having it in the type helps webpack users get autocomplete. That said, this is a minor point — could be revisited if it causes confusion.
| const index = require("../../package/index") | ||
|
|
||
| describe("bundlerUtils", () => { | ||
| describe("isRspack and isWebpack", () => { | ||
| test("exports isRspack as a boolean", () => { | ||
| expect(typeof index.isRspack).toBe("boolean") | ||
| }) | ||
|
|
||
| test("exports isWebpack as a boolean", () => { | ||
| expect(typeof index.isWebpack).toBe("boolean") | ||
| }) | ||
|
|
||
| test("isRspack and isWebpack are mutually exclusive", () => { | ||
| expect(index.isRspack).not.toBe(index.isWebpack) | ||
| }) | ||
|
|
||
| test("defaults to webpack (isWebpack is true, isRspack is false)", () => { | ||
| // Default bundler is webpack | ||
| expect(index.isWebpack).toBe(true) | ||
| expect(index.isRspack).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe("getBundler", () => { | ||
| test("exports getBundler as a function", () => { | ||
| expect(index.getBundler).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns webpack module by default", () => { | ||
| const bundler = index.getBundler() | ||
| expect(bundler).toBeDefined() | ||
| expect(bundler.DefinePlugin).toBeDefined() | ||
| expect(bundler.EnvironmentPlugin).toBeDefined() | ||
| expect(bundler.ProvidePlugin).toBeDefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("getCssExtractPlugin", () => { | ||
| test("exports getCssExtractPlugin as a function", () => { | ||
| expect(index.getCssExtractPlugin).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns MiniCssExtractPlugin by default", () => { | ||
| const CssPlugin = index.getCssExtractPlugin() | ||
| expect(CssPlugin).toBeDefined() | ||
| // MiniCssExtractPlugin has a loader property | ||
| expect(CssPlugin.loader).toBeDefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe("getCssExtractPluginLoader", () => { | ||
| test("exports getCssExtractPluginLoader as a function", () => { | ||
| expect(index.getCssExtractPluginLoader).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns a string loader path", () => { | ||
| const loader = index.getCssExtractPluginLoader() | ||
| expect(typeof loader).toBe("string") | ||
| }) | ||
| }) | ||
|
|
||
| describe("getDefinePlugin", () => { | ||
| test("exports getDefinePlugin as a function", () => { | ||
| expect(index.getDefinePlugin).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns webpack.DefinePlugin by default", () => { | ||
| const DefinePlugin = index.getDefinePlugin() | ||
| expect(DefinePlugin).toBeDefined() | ||
| expect(DefinePlugin).toBeInstanceOf(Function) | ||
| }) | ||
| }) | ||
|
|
||
| describe("getEnvironmentPlugin", () => { | ||
| test("exports getEnvironmentPlugin as a function", () => { | ||
| expect(index.getEnvironmentPlugin).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns webpack.EnvironmentPlugin by default", () => { | ||
| const EnvironmentPlugin = index.getEnvironmentPlugin() | ||
| expect(EnvironmentPlugin).toBeDefined() | ||
| expect(EnvironmentPlugin).toBeInstanceOf(Function) | ||
| }) | ||
| }) | ||
|
|
||
| describe("getProvidePlugin", () => { | ||
| test("exports getProvidePlugin as a function", () => { | ||
| expect(index.getProvidePlugin).toBeInstanceOf(Function) | ||
| }) | ||
|
|
||
| test("returns webpack.ProvidePlugin by default", () => { | ||
| const ProvidePlugin = index.getProvidePlugin() | ||
| expect(ProvidePlugin).toBeDefined() | ||
| expect(ProvidePlugin).toBeInstanceOf(Function) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
All tests run against the default webpack bundler. The rspack branches of getCssExtractPlugin, getCssExtractPluginLoader, and getBundler are never exercised.
Consider adding a describe block like:
describe("rspack bundler path", () => {
let originalBundler
beforeEach(() => {
const config = require("../../package/config")
originalBundler = config.default.assets_bundler
config.default.assets_bundler = "rspack"
jest.resetModules()
})
afterEach(() => {
const config = require("../../package/config")
config.default.assets_bundler = originalBundler
jest.resetModules()
})
// ...assert @rspack/core plugins are returned
})Without this, the core branching logic (the main value of this PR) has no test coverage.
There was a problem hiding this comment.
Correct — rspack path testing is not feasible in this test environment since @rspack/core is not installed. The webpack path tests provide adequate coverage for the utility logic. Rspack-specific behavior is validated in integration tests that have rspack available.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/index.d.ts.template`:
- Around line 13-17: The file is missing a trailing newline at EOF which
violates formatting rules; open the file containing the import of
PluginConstructor, CssExtractPluginConstructor, and BundlerModule from
"./utils/bundlerUtils" and add a single newline character at the end of the file
so the file ends with a newline.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdpackage/index.d.tspackage/index.d.ts.template
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
1 participant
Supersedes #913.
Includes:
Closes #875
Summary by CodeRabbit
New Features
Tests
Changed
Note
Low Risk
Low risk: adds new optional utility exports and TypeScript typings without changing existing bundling behavior; main risk is minor consumer impact if relying on undocumented internals or if required peer deps are missing when calling the new helpers.
Overview
Adds a new
bundlerUtilspublic API to write Webpack/Rspack-agnostic config code, exposingisRspack/isWebpackplus helpers likegetBundler(),getCssExtractPlugin()/getCssExtractPluginLoader(), and plugin constructors (getDefinePlugin(),getEnvironmentPlugin(),getProvidePlugin()).Re-exports these utilities from both
package/index.tsandpackage/rspack/index.ts, updatesindex.d.ts(and template) plusshakapacker/typesto include the new types, and adds Jest coverage and a changelog entry; ESLint overrides are updated to account for the new util file.Written by Cursor Bugbot for commit 363af2c. Configure here.