Add configuration diff engine for comparing bundler configs#961
Add configuration diff engine for comparing bundler configs#961
Conversation
WalkthroughIntroduces a comprehensive configuration diff tool for comparing webpack/rspack configs. Adds CLI entry points, core modules for diffing (DiffEngine, formatter, path normalizer), TypeScript types, documentation, test suites, and refactors shared documentation utilities from configExporter to a central configDocs module. Changes
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant CLI as cli.run()
participant FileLoader as File Loader
participant PathNorm as PathNormalizer
participant DiffEngine as DiffEngine
participant Formatter as DiffFormatter
participant Output as stdout / File
User->>CLI: run(args: string[])
CLI->>CLI: parseArguments(args)
Note over CLI: Extract leftFile, rightFile,<br/>output, format, options
CLI->>FileLoader: loadConfigFile(leftFile)
FileLoader-->>CLI: leftConfig
CLI->>FileLoader: loadConfigFile(rightFile)
FileLoader-->>CLI: rightConfig
alt normalizePaths enabled
CLI->>PathNorm: normalize(leftConfig)
PathNorm-->>CLI: NormalizedConfig
CLI->>PathNorm: normalize(rightConfig)
PathNorm-->>CLI: NormalizedConfig
end
CLI->>DiffEngine: compare(left, right, metadata)
DiffEngine->>DiffEngine: Traverse & compare structures
Note over DiffEngine: Depth limit, ignore rules,<br/>path filtering
DiffEngine-->>CLI: DiffResult
CLI->>Formatter: format[json|yaml|summary|detailed](result)
alt format: "detailed"
Formatter->>Formatter: Side-by-side rendering
Formatter->>Formatter: Documentation lookup per path
Formatter-->>CLI: Rich text output
else format: "json" or "yaml"
Formatter->>Formatter: Serialize to JSON/YAML
Formatter-->>CLI: Structured output
else format: "summary"
Formatter->>Formatter: Compact change counts
Formatter-->>CLI: Summary text
end
alt --output specified
CLI->>Output: writeFile(outputPath, formatted)
else
CLI->>Output: stdout.write(formatted)
end
Output-->>User: Exit code (0 or 1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is ON, but it could not run because Privacy Mode (Legacy) is turned on. To enable Bugbot Autofix, switch your privacy mode in the Cursor dashboard.
|
|
||
| export function hasDocumentation(keyPath: string): boolean { | ||
| return keyPath in WEBPACK_CONFIG_DOCS | ||
| } |
There was a problem hiding this comment.
Duplicate configDocs module across configDiffer and configExporter
Medium Severity
package/configDiffer/configDocs.ts duplicates the purpose and much of the content of the existing package/configExporter/configDocs.ts. Both modules provide webpack/rspack configuration key documentation with overlapping key sets (e.g., mode, devtool, output.path, optimization.minimize, plugins, etc.). Having two separate sources of truth for config documentation creates a maintenance burden and risks them diverging over time.
Greptile SummaryThis PR introduces a Key issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Shim as bin/diff-bundler-config
participant CLI as cli.ts (run)
participant Loader as loadConfigFile
participant Normalizer as PathNormalizer
participant Engine as DiffEngine
participant Formatter as DiffFormatter
User->>Shim: node bin/diff-bundler-config --left=a.yaml --right=b.yaml
Shim->>CLI: run(args)
CLI->>CLI: parseArguments(args)
CLI->>Loader: loadConfigFile(leftFile)
Loader-->>CLI: leftConfig (JSON/YAML/JS/TS)
CLI->>Loader: loadConfigFile(rightFile)
Loader-->>CLI: rightConfig
alt normalizePaths enabled
CLI->>Normalizer: detectBasePath(leftConfig)
CLI->>Normalizer: normalize(leftConfig)
CLI->>Normalizer: normalize(rightConfig)
Normalizer-->>CLI: normalizedLeft, normalizedRight
end
CLI->>Engine: compare(normalizedLeft, normalizedRight, metadata)
Engine->>Engine: compareValues (recursive)
Engine-->>CLI: DiffResult {summary, entries, metadata}
CLI->>Formatter: formatDetailed / formatSummary / formatJson / formatYaml
Formatter-->>CLI: output string
alt --output specified
CLI->>CLI: writeFileSync(outputPath, output)
else stdout
CLI->>User: console.log(output)
end
CLI-->>Shim: exitCode (0 = no diff, 1 = diff found or error)
Shim->>User: process.exit(exitCode)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f556e27d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Review: configDiffer module. The core diff algorithm and documentation are solid, but there is one runtime bug and a few quality issues worth addressing. Details are in the inline comments. Summary: (1) CRITICAL: run() in cli.ts returns number synchronously but the bin shim calls .then() on it - this throws TypeError at runtime. Fix by making run async OR changing the shim to process.exit(run(args)). (2) cli.ts mixes require and import for fs - writeFileSync should be added to the top-level ES import. (3) shouldIgnorePath hardcodes a dot as path separator, breaking --ignore-paths when --path-separator is customized. (4) Array comparison is index-based, causing false-positive change reports when plugins are reordered or inserted - worth documenting as a known limitation. (5) tmp/examples/ files should live in examples/ or docs/examples/, not tmp/. (6) ESLint type-safety overrides in eslint.config.js are applied to cli.ts, formatter.ts, and types.ts but only diffEngine.ts and pathNormalizer.ts truly need them. |
|
Is it just useful for Shakapacker configs in particular, or could it be a separate tool? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (1)
package/configDiffer/cli.ts (1)
76-78: Redundantrequire("fs")—fsis already imported at the top.Line 1 imports
existsSyncandreadFileSyncfromfs. Use the existing import or addwriteFileSyncto the top-level import.♻️ Proposed fix
Update the import at line 1:
-import { existsSync, readFileSync } from "fs" +import { existsSync, readFileSync, writeFileSync } from "fs"Then replace the inline require:
if (options.output) { - const fs = require("fs") - fs.writeFileSync(options.output, output, "utf8") + writeFileSync(options.output, output, "utf8") console.log(`Diff written to: ${options.output}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/configDiffer/cli.ts` around lines 76 - 78, The code redundantly requires "fs" inside the block that writes output; update the top-level import that currently brings in existsSync and readFileSync to also import writeFileSync, then remove the inline const fs = require("fs") and call writeFileSync(options.output, output, "utf8") directly; locate the write path using options.output and the usage of fs.writeFileSync to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/diff-bundler-config`:
- Around line 1-11: The file ends without a trailing newline; update the script
where run(process.argv.slice(2)) is invoked (and the following .then/.catch
block) to ensure the file ends with a single trailing newline character—i.e.,
add a newline at the end of the file after the final closing brace so the last
line terminates properly.
- Around line 6-11: The shim incorrectly treats run as async; update this caller
to handle a synchronous return from run(): invoke run(process.argv.slice(2))
inside a try/catch, capture the returned number (from function run), call
process.exit(returnedNumber) on success, and log error.message then
process.exit(1) in the catch block; reference the run(...) call and the existing
process.exit/error handling in this file to locate where to replace the
.then/.catch usage.
In `@docs/config-diff.md`:
- Around line 329-351: The fenced code block containing the CLI usage starting
with "bin/diff-bundler-config --left=<file1> --right=<file2> [options]" lacks a
language specifier; update the opening fence from ``` to ```text (or another
appropriate language) so markdownlint renders it correctly and avoids warnings.
- Around line 1-436: The file is missing a trailing newline; open the
docs/config-diff.md content and add a single newline character at the end of the
file (after the final "Troubleshooting Guide" / "See Also" section) so the file
ends with a newline per project coding guidelines.
- Around line 369-381: The example incorrectly passes a unused format option to
DiffEngine; update the snippet to remove format from the DiffEngine constructor
and demonstrate the correct two-step flow: call DiffEngine.compare(config1,
config2) to get result, then instantiate DiffFormatter and call the appropriate
formatter method (e.g., DiffFormatter.formatJson(result)) to produce JSON
output, referencing DiffEngine, DiffEngine.compare, and DiffFormatter.formatJson
so readers know where formatting is handled.
In `@lib/install/bin/diff-bundler-config`:
- Around line 1-11: Add a trailing newline at the end of the file containing the
minimal shim (the script invoking run(process.argv.slice(2)) and the .catch(...)
block) so the file ends with a newline character; open the file
diff-bundler-config and ensure the final line is followed by a single '\n'
before saving/committing.
In `@package/configDiffer/cli.ts`:
- Around line 138-140: The CLI parsing for "--max-depth=" uses parseInt without
validating the result (see parseValue and assignment to options.maxDepth), which
can set NaN when given non-numeric input; update the handler to validate the
parsed depth: call parseValue(arg,"--max-depth="), if the string equals "null"
set options.maxDepth = null, else parse to an integer and check isNaN(parsed) —
if NaN, produce a clear error (throw or print and exit) or fallback to a safe
default, otherwise assign the numeric value to options.maxDepth; ensure all
branches use the symbols parseValue and options.maxDepth so the fix is applied
in the correct CLI parsing block.
- Around line 173-192: The JS/TS loader currently only unwraps default objects
but does not handle function exports, so when a config file exports (env, argv)
=> config you end up with a function instead of a resolved config object; update
the logic after require(resolvedPath) to: if the resolved value (or its
.default) is a function, invoke it with CLI-provided env/argv (or empty objects
if none) and use the returned value as the loaded config; keep the existing
steps that delete require.cache and unwrap .default, and reference the local
variable name loaded as the place to add the typeof loaded === "function" check
and invocation (optionally use CLI flags like env/argv passed into this module
to supply arguments).
- Around line 16-89: The run function is synchronous (declared to return number)
but the bin shim (bin/diff-bundler-config) calls run(...).then(...), causing a
TypeError; fix this by making run asynchronous so it returns a Promise<number>:
change the signature of run (the exported function) to async function run(args:
string[]): Promise<number> (or otherwise return a Promise), keeping the existing
logic and return values intact so callers using .then() work as expected.
In `@package/configDiffer/configDocs.ts`:
- Around line 1-167: Add a trailing newline at the end of the file and export
the ConfigDoc interface so downstream consumers can type-check docs;
specifically, change "interface ConfigDoc" to "export interface ConfigDoc" and
ensure the file ends with a single newline character after the existing
exports/getters (getDocForKey, hasDocumentation, and WEBPACK_CONFIG_DOCS).
In `@package/configDiffer/diffEngine.ts`:
- Around line 87-89: The code currently treats any non-Array/RegExp/Date object
as a plain object and recurses via compareObjects, which hides differences
between arbitrary class instances; change the recursion guard in
isObject/compare call sites so that you only deep-compare plain objects (e.g.,
Object.getPrototypeOf(x) === Object.prototype or x.constructor === Object)
before calling compareObjects from the spots that currently call it (the
isObject check and the other recursion sites in diffEngine.ts that invoke
compareObjects); if the two values are objects but have different constructors,
do not recurse—emit a difference based on constructor/type (or a shallow
inequality) instead. Ensure the same constructor/plain-object check is added to
every other block that currently descends into objects (the other compareObjects
invocation sites reported in the review) so class instances are compared by
constructor/type rather than by enumerating internals.
- Around line 162-173: The shouldIgnorePath logic fails when ignorePatterns
contain characters that are special in regex and it hard-codes "." for
descendant checks; update shouldIgnorePath (and rely on createHumanPath) to (1)
use the configured path separator (e.g., this.options.pathSeparator or
this.pathSeparator) instead of the literal "." when checking startsWith, (2)
escape all regex metacharacters in the ignorePath before transforming '*' into
'.*' (implement an escapeRegExp helper or use a safe escape) and then build the
RegExp, and (3) wrap RegExp creation in a try/catch or validate the pattern so
malformed patterns don’t throw; reference shouldIgnorePath, createHumanPath, and
options.ignorePaths when locating the code to change.
In `@package/configDiffer/formatter.ts`:
- Around line 164-165: getDocForKey is doing an exact lookup against
entry.path.humanPath (e.g., "optimization.minimizer.[0]") which fails for
array-backed keys; modify the logic in formatter.ts where doc is assigned (the
getDocForKey call for entry.path.humanPath) to first normalize/walk the path by
removing any array index segments (like "[0]", "[1]") and then iteratively walk
up parent segments (e.g., try "optimization.minimizer", then "optimization")
until getDocForKey returns a non-null value; keep using getDocForKey for each
candidate but ensure you try the nearest parent documented key before falling
back to null.
- Around line 97-98: The label derivation uses filename.split("/") which fails
on Windows paths; change it to use the Node path utilities (e.g., use
path.basename on filename) so basename correctly handles both POSIX and Windows
paths, then apply the existing withoutExt replacement (on the basename variable)
to strip .yaml/.yml/.json/.js/.ts extensions; ensure you import/require the path
module and replace the split/pop logic in formatter.ts where basename and
withoutExt are defined.
In `@package/configDiffer/pathNormalizer.ts`:
- Around line 41-55: normalizePath currently uses platform-default path helpers
causing Windows/UNC/home-prefixed paths to be mangled; update normalizePath (and
any detectBasePath usage) to detect input format (Windows vs POSIX vs
home-prefixed) and use the corresponding path module (path.win32 or path.posix)
for isAbsolute/resolve/relative/sep operations, explicitly expand and normalize
leading "~/" to the user's home path before computing relative paths, and handle
the empty-relative case (when relative(...) === "") by returning "./" instead of
the original absolute string; also ensure the file ends with a newline.
In `@package/configDiffer/types.ts`:
- Around line 1-48: Add a single trailing newline character at the end of the
file containing the DiffOperation, DiffPath, DiffEntry, DiffResult, DiffOptions,
and NormalizedConfig type definitions so the file ends with a newline (ensure
the final byte is a newline character); this satisfies the coding guideline
"ALWAYS end all files with a trailing newline" without changing any type
declarations.
In `@scripts/ci-local.sh`:
- Around line 1-66: Add a trailing newline character to the end of the script so
the file ends with a newline (POSIX-compliant). Open the script containing set
-e, the run_check function and FAILED_CHECKS array and ensure there is a single
newline character after the final if/fi block (after the exit statement) so the
file ends with a trailing newline.
In `@test/configDiffer/diffEngine.test.js`:
- Around line 207-213: The test currently only checks that
result.metadata.comparedAt yields a Date object, which is insufficient; update
the assertion in the test for DiffEngine.compare to verify the timestamp string
is parseable as a valid date by validating result.metadata.comparedAt with
Date.parse (or new Date(...).getTime()) and asserting it is not NaN so invalid
strings like "not-a-date" will fail; locate the test around the DiffEngine
instantiation and result = engine.compare({ a: 1 }, { a: 1 }) and replace the
second expect to assert parseability of result.metadata.comparedAt.
In `@tmp/examples/dev-config.yaml`:
- Around line 1-18: The file (tmp/examples/dev-config.yaml) is missing a
trailing newline; open the YAML file (containing keys like mode, devtool,
output, cache, optimization, plugins, devServer) and ensure you add a single
newline character at the end of the file so it ends with a trailing EOL.
In `@tmp/examples/prod-config.yaml`:
- Around line 1-19: The file is missing a trailing newline at EOF; open the
prod-config.yaml content (containing keys like mode, devtool, output, cache,
optimization, plugins) and ensure the file ends with a single newline character
by adding a trailing line break after the last line so the file conforms to the
"end with a trailing newline" guideline.
---
Nitpick comments:
In `@package/configDiffer/cli.ts`:
- Around line 76-78: The code redundantly requires "fs" inside the block that
writes output; update the top-level import that currently brings in existsSync
and readFileSync to also import writeFileSync, then remove the inline const fs =
require("fs") and call writeFileSync(options.output, output, "utf8") directly;
locate the write path using options.output and the usage of fs.writeFileSync to
make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d21ad74-8968-4986-8865-c8de123e99d2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
README.mdbin/diff-bundler-configdocs/config-diff.mdeslint.config.jslib/install/bin/diff-bundler-configpackage.jsonpackage/configDiffer/cli.tspackage/configDiffer/configDocs.tspackage/configDiffer/diffEngine.tspackage/configDiffer/formatter.tspackage/configDiffer/index.tspackage/configDiffer/pathNormalizer.tspackage/configDiffer/types.tsscripts/ci-local.shtest/configDiffer/diffEngine.test.jstest/configDiffer/formatter.test.jstest/configDiffer/pathNormalizer.test.jstmp/examples/dev-config.yamltmp/examples/prod-config.yaml
Implements Phase 1 of issue #667: Core Diff Engine Features: - Deep object diff algorithm with path-level granularity - Support for nested objects, arrays, and special types (functions, RegExp, Date) - Path normalization utilities for comparing absolute paths - Multiple output formats: detailed, summary, JSON, YAML - Advanced filtering options: ignore keys, ignore paths (with wildcards) - Comprehensive test coverage with 39 passing tests New CLI command: - bin/diff-bundler-config: Compare two config files - Supports JSON, YAML, JS, and TS config files - Exit code 0 if no differences, 1 if differences found Documentation: - Complete diff tool guide in docs/config-diff.md - Updated README.md with diff examples - Usage examples and tips 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add configDiffer to package.json exports for proper module resolution - Add comprehensive 'Why use this?' section explaining advantages over visual diff - Document 5 common troubleshooting workflows with examples - Explain integration with --doctor mode - Add practical guidance on what to look for in different scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ation
Address code review feedback:
1. Fix basePath detection to use separate paths for left/right configs
- Each side now detects its own base path independently
- More accurate normalization when comparing configs from different locations
2. Remove no-op map in createHumanPath
- Simplified path formatting by removing pointless transformation
- Cleaner, more maintainable code
3. Fix looksLikePath to avoid normalizing URLs and module specifiers
- Explicitly exclude URL schemes (http://, webpack://, etc.)
- Exclude npm module specifiers starting with @
- Only match actual filesystem paths (absolute, relative, home, Windows drives)
- Prevents incorrect normalization of webpack:// URLs and @scope/package names
4. Replace summary/detailed formats with contextual format
- Shows "What it does" explanations for webpack config keys
- Includes impact analysis (e.g., "Enabling production optimizations")
- Links to official webpack documentation
- Displays default values where applicable
- Much more useful for understanding WHY something changed
5. Add comprehensive configuration documentation database
- 20+ documented webpack/rspack configuration keys
- Covers mode, devtool, optimization, output, devServer, etc.
- Extensible for future additions
New output format example:
1. [~] mode
What it does:
Defines the environment mode (development, production, or none).
Controls built-in optimizations and defaults.
Affects: Minification, tree-shaking, source maps, and performance optimizations
Old value: "development"
New value: "production"
Impact: Enabling production optimizations (minification, tree-shaking)
Documentation: https://webpack.js.org/configuration/mode/
Summary format is now concise: "8 changes: +3 -1 ~4"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Changes based on feedback:
1. Remove redundant value type display
- Type information doesn't add value to the diff output
- Users can see the type from the value itself
2. Replace "old/new" with file-based labels
- Extract meaningful short names from filenames
- webpack-development-client.yaml -> "dev-client"
- webpack-production-server.yaml -> "prod-server"
- Falls back to "left"/"right" if no pattern matches
3. Show values in a clearer format:
Before:
Old value: "development"
New value: "production"
After:
Values:
dev-client: "development"
prod-client: "production"
4. Design supports future N-way comparison
- Format naturally extends to comparing 3+ configs
- Could show dev-client, dev-server, prod-client, prod-server all in one view
- Labels make it clear which value comes from which file
Example output:
3. [~] mode
What it does:
Defines the environment mode (development, production, or none).
Affects: Minification, tree-shaking, source maps
Values:
dev: "development"
prod: "production"
Impact: Enabling production optimizations
Much clearer which config has which value!
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
- Run prettier --write on all files to fix formatting - Add scripts/ci-local.sh for reproducing CI checks locally - Inspired by react_on_rails approach to local CI validation - Install missing dependencies (knip, ts-jest) This addresses CI failures for prettier formatting checks.
Fix code style issues (no-else-return, no-continue, no-plusplus, prefer-template, prefer-const, no-nested-ternary, lines-between-class-members, unused imports/vars, unnecessary async) and add ESLint config overrides for type-safety rules matching the existing configExporter pattern, since the config differ inherently works with arbitrary untyped config objects. Co-Authored-By: Claude Opus 4.6 <[email protected]>
justin808
force-pushed
the
config-diff-engine
branch
from
b0f556e to
f00256c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f00256c11a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eslint.config.js (1)
232-249: Scope this override to concrete files.The
package/configDiffer/**/*.tsglob also disables these rules forindex.tsand any future file added under this folder, even if it does not need the escape hatch. Matching the explicit-file approach used in the next block keeps new code from silently inheriting weaker linting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 232 - 249, The override currently targets the broad glob "package/configDiffer/**/*.ts" and unintentionally disables rules for index.ts and any new files; narrow this to the concrete files that require the exceptions by replacing that glob with an explicit list of known filenames (e.g., the specific modules like diffEngine.ts, pathNormalizer.ts, cli.ts, formatter.ts or whichever files in the configDiffer folder actually need the rules off) so the rules ("@typescript-eslint/no-use-before-define", "import/no-dynamic-require", "class-methods-use-this", "import/prefer-default-export") only apply to those named files and not to index.ts or future additions.package/configDiffer/pathNormalizer.ts (1)
144-146: Unreachable code: function check is dead.The
typeof value === "function"check is unreachable becausetypeofreturns"function"(not"object") for functions, so they already returnfalseat line 132.♻️ Suggested removal
if (value instanceof Date || value instanceof RegExp) { return false } - if (typeof value === "function") { - return false - } - return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/configDiffer/pathNormalizer.ts` around lines 144 - 146, Remove the redundant unreachable check "if (typeof value === \"function\") { return false }" that tests the local variable value, since functions are already handled earlier and this branch is dead; delete that conditional from pathNormalizer.ts (leave surrounding logic intact) and run tests to confirm no behavioral change.
🤖 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/configDiffer/diffEngine.ts`:
- Around line 14-23: DiffEngine's constructor accepts DiffOptions.normalizePaths
but compare() never applies it, so programmatic callers still get
platform-specific path diffs; update the DiffEngine.compare method to, when
this.options.normalizePaths is true, normalize both input trees (and any
path-like keys/values used for matching) before diffing using
this.options.pathSeparator, and also normalize entries in
this.options.ignorePaths/ignoreKeys so comparisons and path matching use the
same canonical separator; modify the compare implementation (and any helper it
calls) to perform this pre-processing step on left/right inputs and ignore lists
prior to the existing diff logic.
In `@package/configDiffer/formatter.ts`:
- Around line 237-275: analyzeImpact uses entry.path.humanPath which is
presentation-dependent and breaks dot-key matching when a custom separator is
used; change it to derive a stable dot-separated key via
entry.path.path.join(".") (so comparisons like "optimization.minimize",
"devtool", "output.filename", and "mode" continue to match regardless of
presentation settings) and update any variable name (currently path) accordingly
so the existing conditionals keep working.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 232-249: The override currently targets the broad glob
"package/configDiffer/**/*.ts" and unintentionally disables rules for index.ts
and any new files; narrow this to the concrete files that require the exceptions
by replacing that glob with an explicit list of known filenames (e.g., the
specific modules like diffEngine.ts, pathNormalizer.ts, cli.ts, formatter.ts or
whichever files in the configDiffer folder actually need the rules off) so the
rules ("@typescript-eslint/no-use-before-define", "import/no-dynamic-require",
"class-methods-use-this", "import/prefer-default-export") only apply to those
named files and not to index.ts or future additions.
In `@package/configDiffer/pathNormalizer.ts`:
- Around line 144-146: Remove the redundant unreachable check "if (typeof value
=== \"function\") { return false }" that tests the local variable value, since
functions are already handled earlier and this branch is dead; delete that
conditional from pathNormalizer.ts (leave surrounding logic intact) and run
tests to confirm no behavioral change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06ec5678-dedc-44eb-a260-4a667920e891
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
README.mdbin/diff-bundler-configdocs/config-diff.mdeslint.config.jslib/install/bin/diff-bundler-configpackage.jsonpackage/configDiffer/cli.tspackage/configDiffer/configDocs.tspackage/configDiffer/diffEngine.tspackage/configDiffer/formatter.tspackage/configDiffer/index.tspackage/configDiffer/pathNormalizer.tspackage/configDiffer/types.tsscripts/ci-local.shtest/configDiffer/diffEngine.test.jstest/configDiffer/formatter.test.jstest/configDiffer/pathNormalizer.test.jstmp/examples/dev-config.yamltmp/examples/prod-config.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- tmp/examples/dev-config.yaml
- bin/diff-bundler-config
- tmp/examples/prod-config.yaml
- test/configDiffer/formatter.test.js
- package/configDiffer/index.ts
- package/configDiffer/types.ts
- test/configDiffer/pathNormalizer.test.js
- package.json
- lib/install/bin/diff-bundler-config
- docs/config-diff.md
| constructor(options: DiffOptions = {}) { | ||
| this.options = { | ||
| includeUnchanged: options.includeUnchanged ?? false, | ||
| maxDepth: options.maxDepth ?? null, | ||
| ignoreKeys: options.ignoreKeys ?? [], | ||
| ignorePaths: options.ignorePaths ?? [], | ||
| format: options.format ?? "detailed", | ||
| normalizePaths: options.normalizePaths ?? true, | ||
| pathSeparator: options.pathSeparator ?? "." | ||
| } |
There was a problem hiding this comment.
normalizePaths is currently a no-op on DiffEngine.
DiffOptions exposes this flag and DiffEngine is part of the public API, but compare() never normalizes its inputs. Right now only package/configDiffer/cli.ts does that pre-processing, so programmatic callers still get machine-specific path diffs even when they opt in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/configDiffer/diffEngine.ts` around lines 14 - 23, DiffEngine's
constructor accepts DiffOptions.normalizePaths but compare() never applies it,
so programmatic callers still get platform-specific path diffs; update the
DiffEngine.compare method to, when this.options.normalizePaths is true,
normalize both input trees (and any path-like keys/values used for matching)
before diffing using this.options.pathSeparator, and also normalize entries in
this.options.ignorePaths/ignoreKeys so comparisons and path matching use the
same canonical separator; modify the compare implementation (and any helper it
calls) to perform this pre-processing step on left/right inputs and ignore lists
prior to the existing diff logic.
| private analyzeImpact(entry: DiffEntry): string | null { | ||
| const path = entry.path.humanPath | ||
| const newVal = entry.newValue | ||
|
|
||
| // Specific impact analysis | ||
| if (path === "mode") { | ||
| if (newVal === "production") { | ||
| return "Enabling production optimizations (minification, tree-shaking)" | ||
| } | ||
| if (newVal === "development") { | ||
| return "Enabling development features (better error messages, faster builds)" | ||
| } | ||
| } | ||
|
|
||
| if (path === "optimization.minimize") { | ||
| if (newVal === true || newVal === "true") { | ||
| return "Code will be minified - smaller bundles but slower builds" | ||
| } | ||
| return "Minification disabled - larger bundles but faster builds" | ||
| } | ||
|
|
||
| if (path === "devtool") { | ||
| if (newVal === "source-map") { | ||
| return "Full source maps - best debugging but slower builds" | ||
| } | ||
| if (newVal === "eval") { | ||
| return "Fastest builds but poor debugging experience" | ||
| } | ||
| if (String(newVal).includes("cheap") || String(newVal).includes("eval")) { | ||
| return "Faster builds with some debugging capability" | ||
| } | ||
| } | ||
|
|
||
| if (path.includes("output.filename")) { | ||
| if (String(newVal).includes("[contenthash]")) { | ||
| return "Cache busting enabled - better long-term caching" | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Drive impact analysis from the structural path, not humanPath.
analyzeImpact() matches dot-separated keys, so --path-separator=/ turns optimization.minimize into optimization/minimize and the built-in impact hints silently stop working for nested keys. Use entry.path.path.join(".") here so presentation settings do not change the semantics.
♻️ Minimal fix
- const path = entry.path.humanPath
+ const path = entry.path.path.join(".")📝 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.
| private analyzeImpact(entry: DiffEntry): string | null { | |
| const path = entry.path.humanPath | |
| const newVal = entry.newValue | |
| // Specific impact analysis | |
| if (path === "mode") { | |
| if (newVal === "production") { | |
| return "Enabling production optimizations (minification, tree-shaking)" | |
| } | |
| if (newVal === "development") { | |
| return "Enabling development features (better error messages, faster builds)" | |
| } | |
| } | |
| if (path === "optimization.minimize") { | |
| if (newVal === true || newVal === "true") { | |
| return "Code will be minified - smaller bundles but slower builds" | |
| } | |
| return "Minification disabled - larger bundles but faster builds" | |
| } | |
| if (path === "devtool") { | |
| if (newVal === "source-map") { | |
| return "Full source maps - best debugging but slower builds" | |
| } | |
| if (newVal === "eval") { | |
| return "Fastest builds but poor debugging experience" | |
| } | |
| if (String(newVal).includes("cheap") || String(newVal).includes("eval")) { | |
| return "Faster builds with some debugging capability" | |
| } | |
| } | |
| if (path.includes("output.filename")) { | |
| if (String(newVal).includes("[contenthash]")) { | |
| return "Cache busting enabled - better long-term caching" | |
| } | |
| } | |
| private analyzeImpact(entry: DiffEntry): string | null { | |
| const path = entry.path.path.join(".") | |
| const newVal = entry.newValue | |
| // Specific impact analysis | |
| if (path === "mode") { | |
| if (newVal === "production") { | |
| return "Enabling production optimizations (minification, tree-shaking)" | |
| } | |
| if (newVal === "development") { | |
| return "Enabling development features (better error messages, faster builds)" | |
| } | |
| } | |
| if (path === "optimization.minimize") { | |
| if (newVal === true || newVal === "true") { | |
| return "Code will be minified - smaller bundles but slower builds" | |
| } | |
| return "Minification disabled - larger bundles but faster builds" | |
| } | |
| if (path === "devtool") { | |
| if (newVal === "source-map") { | |
| return "Full source maps - best debugging but slower builds" | |
| } | |
| if (newVal === "eval") { | |
| return "Fastest builds but poor debugging experience" | |
| } | |
| if (String(newVal).includes("cheap") || String(newVal).includes("eval")) { | |
| return "Faster builds with some debugging capability" | |
| } | |
| } | |
| if (path.includes("output.filename")) { | |
| if (String(newVal).includes("[contenthash]")) { | |
| return "Cache busting enabled - better long-term caching" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/configDiffer/formatter.ts` around lines 237 - 275, analyzeImpact uses
entry.path.humanPath which is presentation-dependent and breaks dot-key matching
when a custom separator is used; change it to derive a stable dot-separated key
via entry.path.path.join(".") (so comparisons like "optimization.minimize",
"devtool", "output.filename", and "mode" continue to match regardless of
presentation settings) and update any variable name (currently path) accordingly
so the existing conditionals keep working.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f874456f1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const expandedPath = this.expandHomePath(value) | ||
| if (this.isAbsolutePath(expandedPath)) { | ||
| const flavor = this.detectPathFlavor(expandedPath) | ||
| pathsByFlavor[flavor].push(expandedPath) | ||
| } |
There was a problem hiding this comment.
detectBasePath currently treats every absolute-looking string as a filesystem path candidate. In real webpack configs, output.publicPath is often a URL root like /packs/; combining that with real paths such as /app/public/packs drives the common prefix down to /, so normalization keeps host-specific prefixes (for example ./home/ci/... vs ./Users/...) instead of producing stable project-relative paths. That breaks the main cross-environment normalization behavior this tool advertises.
Useful? React with 👍 / 👎.
| console.error(`Error: ${error.message}`) | ||
| return 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Exit code 1 is returned both when differences are found (expected) and when an error is thrown (unexpected). CI users cannot distinguish between these cases. Consider returning exit code 2 for errors so pipelines can tell the difference between 'diffs exist' and 'tool failed'.
| --left=<file> Path to the first (left) config file | ||
| --right=<file> Path to the second (right) config file | ||
|
|
||
| Output Options: |
There was a problem hiding this comment.
Executing arbitrary JS/TS files via require() is a significant security consideration. The CLI help text (showHelp()) should include a warning that .js/.ts files are executed, not just read — so users don't accidentally run untrusted configs.
| console.log(output) | ||
| } | ||
|
|
||
| return result.summary.totalChanges > 0 ? 1 : 0 |
There was a problem hiding this comment.
The exit code conflates two very different outcomes — "configs differ" and "unexpected error" — both map to 1. This makes it impossible for a CI script to distinguish between an expected diff and a tool crash. Consider using a third exit code (e.g. 2) for runtime errors, which is a common UNIX convention:
| return result.summary.totalChanges > 0 ? 1 : 0 | |
| return result.summary.totalChanges > 0 ? 1 : 0 | |
| } catch (error: unknown) { | |
| const message = error instanceof Error ? error.message : String(error) | |
| console.error(`Error: ${message}`) | |
| return 2 |
The docs would then say:
0— identical configs1— differences found2— error (missing file, bad format, etc.)
| } | ||
|
|
||
| const ext = extname(resolvedPath).toLowerCase() | ||
| const content = readFileSync(resolvedPath, "utf8") |
There was a problem hiding this comment.
Unrecognised arguments are silently ignored. A user who writes --format detailed (space-separated, not =-separated) will silently get the default format with no warning. Consider emitting a warning or error for anything that looks like a flag but isn't matched:
} else if (arg.startsWith("--") || arg.startsWith("-")) {
throw new Error(
`Unknown option: '${arg}'. Use --help for usage information.`
)
}This prevents typos like --Format=json or --no-normalize (vs --no-normalize-paths) from going unnoticed.
| } | ||
|
|
||
| function showHelp(): void { | ||
| console.log(` |
There was a problem hiding this comment.
Two notes on the .js/.ts loading path:
-
Global side effect:
require("ts-node/register/transpile-only")installs a permanent, process-global require hook. Sincerun()is a public export inindex.ts, programmatic callers will have their entirerequirepipeline mutated after the first.tsconfig is loaded. At minimum add a comment documenting this. -
Arbitrary code execution: Loading and executing user-specified
.js/.tsfiles is expected for a config-loading tool, but worth noting in the docs (the config file will execute in the same Node process with the same permissions). The existing docs mention it requires the file to "export a config object" but don't call out that arbitrary code executes at load time (e.g. side-effectful plugins in the config factory).
| lines.push(` ${rightLabel}: <not set>`) | ||
| } else if (entry.operation === "changed") { | ||
| lines.push(` ${leftLabel}: ${this.formatValue(entry.oldValue)}`) | ||
| lines.push(` ${rightLabel}: ${this.formatValue(entry.newValue)}`) |
There was a problem hiding this comment.
The comment "Keep formatDetailed as an alias for backward compatibility" implies there are existing callers — but this is brand-new code in this PR. There's no backward compatibility to maintain yet. The comment is misleading and suggests the API design wasn't settled before being written.
If the intent is that formatContextual is the real implementation and formatDetailed is just the CLI-facing name, consider reversing the naming (make formatDetailed primary and formatContextual the descriptive name) or just remove the alias and use the same name throughout.
| @@ -0,0 +1,163 @@ | |||
| // Configuration documentation for webpack/rspack keys | |||
There was a problem hiding this comment.
All documentationUrl values point to webpack.js.org, but the tool is also marketed for rspack comparisons. rspack has a compatible but distinct API surface and its own docs at rspack.dev. This can mislead rspack users who follow the links.
Consider either:
- Omitting
documentationUrlfor now and adding it only once the tool has separate rspack-aware docs lookup - Noting in the description that the links are webpack-specific
- Renaming
WEBPACK_CONFIG_DOCSand making the lookup bundler-aware
| return { | ||
| summary, | ||
| entries: this.entries, | ||
| metadata: { |
There was a problem hiding this comment.
The returned DiffResult.entries is a direct reference to this.entries. While calling compare() a second time does reassign this.entries = [] (not mutate it), so previously returned results are safe, this is subtle. It also means all DiffResult objects from the same engine share no backing array, but a user who stores a result and then adds to the array externally would see mutations reflected in the engine if they called compare() before the reassignment.
To make ownership unambiguous, return a copy:
entries: [...this.entries],
Review of PR #961: Add configuration diff engine for comparing bundler configsOverall, this is a well-structured addition with good documentation and test coverage. The modular design (DiffEngine, DiffFormatter, PathNormalizer, CLI) is clean. Below are the key issues I found. CRITICAL ISSUESExit code ambiguity makes CI integration fragile The docs state: Arbitrary code execution from JS/TS config files
MODERATE ISSUES
The
In
MINOR ISSUES
This is new code with no prior version to be compatible with. The comment implies a prior API existed, which will confuse future readers. TypeScript safety rules disabled too broadly Disabling |
Review: Add configuration diff engineThis is a well-structured addition with clear module separation (engine, formatter, normalizer, CLI) and good test coverage. The path normalization logic in particular is thorough. A few issues to address before merging: Bugs / Correctness
API / UX
Design / Maintainability
Scope concern This adds ~4 000 LOC. The CLAUDE.md guidelines note that every config option is maintenance surface and caution against features that add complexity for everyone. The configDocs lookup, per-key impact analysis strings, and multi-format output all represent ongoing maintenance. Consider scoping Phase 1 to the core diff engine plus JSON/YAML output, and treating the contextual docs and impact analysis as a follow-up once the basic tool proves useful to users. |
| console.log(output) | ||
| } | ||
|
|
||
| return result.summary.totalChanges > 0 ? 1 : 0 |
There was a problem hiding this comment.
Exit code conflation: both "differences found" and "error occurred" return 1, making it impossible for CI scripts to distinguish the two scenarios.
| return result.summary.totalChanges > 0 ? 1 : 0 | |
| return result.summary.totalChanges > 0 ? 1 : 0 |
Consider using exit code 2 for runtime errors (in the catch block below) so consumers can differentiate:
bin/diff-bundler-config --left=a.yaml --right=b.yaml
case $? in
0) echo "Configs identical — OK" ;;
1) echo "Differences found" ;;
2) echo "Tool error — check output" ;;
esacThe docs should reflect whichever convention is chosen.
| export class DiffEngine { | ||
| private options: Required<DiffOptions> | ||
|
|
||
| private entries: DiffEntry[] = [] |
There was a problem hiding this comment.
Instance-level mutable state that's reset on every compare() call is a surprising design. If a caller reuses a DiffEngine instance (which the programmatic API docs encourage), the entries from a previous comparison are silently discarded.
Consider making entries local to compare() and threading it through the private methods, which would make the class stateless and easier to reason about:
| private entries: DiffEntry[] = [] | |
| private options: Required<DiffOptions> |
Then in compare():
compare(left: any, right: any, metadata?: any): DiffResult {
const entries: DiffEntry[] = []
this.compareValues(left, right, [], 0, entries)
const summary = this.calculateSummary(entries)
...
}
| } | ||
| } | ||
|
|
||
| private compareValues( |
There was a problem hiding this comment.
No circular-reference protection. Webpack configs that contain plugin instances (e.g. new webpack.DefinePlugin(...)) can hold back-references to the compiler or other circular structures. If a JS config file is passed, compareValues will infinitely recurse until a stack overflow.
Consider tracking visited object references with a WeakSet:
private compareValues(
left: any,
right: any,
path: string[],
depth: number = 0,
visited = new WeakSet()
): void {
// Early exit on circular refs
if (typeof left === 'object' && left !== null) {
if (visited.has(left)) return
visited.add(left)
}
...
}Alternatively, the existing maxDepth option mitigates this for YAML/JSON inputs (where circular refs are impossible), but the JS/TS loading path needs protection.
| return withoutExt | ||
| } | ||
|
|
||
| // Keep formatDetailed as an alias for backward compatibility |
There was a problem hiding this comment.
The "backward compatibility" rationale doesn't apply here — both formatDetailed and formatContextual are brand-new in this PR. The comment is misleading.
| // Keep formatDetailed as an alias for backward compatibility | |
| formatDetailed(result: DiffResult): string { | |
| return this.formatContextual(result) | |
| } |
If you want formatDetailed to remain the primary public name (since that's what the CLI uses), consider flipping the delegation: keep formatDetailed as the implementation and make formatContextual the alias (or remove it if unused externally).
| } | ||
|
|
||
| if (options.output) { | ||
| writeFileSync(options.output, output, "utf8") |
There was a problem hiding this comment.
If the directory for options.output doesn't exist, writeFileSync will throw a ENOENT error. That exception is caught by the outer try/catch and returns exit code 1 — indistinguishable from "differences found".
A small guard improves the error message:
import { existsSync, mkdirSync } from "fs"
import { dirname } from "path"
const outDir = dirname(options.output)
if (!existsSync(outDir)) {
throw new Error(`Output directory does not exist: ${outDir}`)
}
writeFileSync(options.output, output, "utf8")
ReviewThe overall implementation is clean and well-structured. The module separation (engine, formatter, normalizer, CLI) is good, the TypeScript types are reasonable, and the 39-test suite covers the key paths. A few issues worth addressing before merge: Bugs / Design Issues 1. Circular reference protection missing (see inline comment on diffEngine.ts:42) 2. Exit code conflation (see inline comment on cli.ts:83) 3. Stateful DiffEngine (see inline comment on diffEngine.ts:12) 4. Output directory not validated (see inline comment on cli.ts:77) Code Quality 5. Misleading backward-compatibility comment (see inline comment on formatter.ts:134) Missing 6. No CHANGELOG entry Minor Notes
|
…cs.ts Both configDiffer and configExporter had separate configDocs modules with overlapping webpack/rspack documentation. Merged into a single shared module with the richer ConfigDoc structure and all keys from both sources. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package/configDiffer/formatter.ts (1)
237-277:⚠️ Potential issue | 🟡 MinorUse structural path for impact analysis, not presentation path.
analyzeImpact()compares against dot-separated keys like"mode","optimization.minimize", etc. However, it usesentry.path.humanPathwhich is affected by the--path-separatorCLI option. If a user runs with--path-separator=/, thehumanPathbecomesoptimization/minimizeand none of the impact hints will match.Use the structural path instead:
Proposed fix
private analyzeImpact(entry: DiffEntry): string | null { - const path = entry.path.humanPath + const path = entry.path.path.join(".") const newVal = entry.newValue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/configDiffer/formatter.ts` around lines 237 - 277, analyzeImpact() is comparing against dot-separated structural keys but currently reads entry.path.humanPath which varies with the CLI path-separator; replace that with the structural path (e.g. use the path segments or structural path property on entry.path instead of humanPath), for example: derive a dot-joined structuralPath from entry.path (e.g., entry.path.segments.join('.') or entry.path.path.join('.')) and use that in place of entry.path.humanPath so comparisons like "optimization.minimize" and "output.filename" always match regardless of --path-separator; keep all existing string comparisons in analyzeImpact unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package/configDiffer/formatter.ts`:
- Around line 237-277: analyzeImpact() is comparing against dot-separated
structural keys but currently reads entry.path.humanPath which varies with the
CLI path-separator; replace that with the structural path (e.g. use the path
segments or structural path property on entry.path instead of humanPath), for
example: derive a dot-joined structuralPath from entry.path (e.g.,
entry.path.segments.join('.') or entry.path.path.join('.')) and use that in
place of entry.path.humanPath so comparisons like "optimization.minimize" and
"output.filename" always match regardless of --path-separator; keep all existing
string comparisons in analyzeImpact unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 188b2fca-fa1a-4c0d-ac15-78d0a0bad831
📒 Files selected for processing (14)
eslint.config.jspackage/configDiffer/cli.tspackage/configDiffer/formatter.tspackage/configDiffer/pathNormalizer.tspackage/configDiffer/types.tspackage/configDocs.tspackage/configExporter/configDocs.tspackage/configExporter/index.d.tspackage/configExporter/index.tspackage/configExporter/yamlSerializer.tsspec/fixtures/configDiffer/dev-config.yamlspec/fixtures/configDiffer/prod-config.yamltest/configDiffer/formatter.test.jstest/configDiffer/pathNormalizer.test.js
💤 Files with no reviewable changes (1)
- package/configExporter/configDocs.ts
✅ Files skipped from review due to trivial changes (1)
- test/configDiffer/pathNormalizer.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package/configDiffer/types.ts
| humanPath.startsWith(`${ignorePath}${this.options.pathSeparator}`) | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Performance: RegExp compiled on every traversal step
shouldIgnorePath is called for every node in the config tree. Each call with a wildcard ignorePath constructs a new RegExp object. For a config with hundreds of nodes and several wildcard patterns, this creates thousands of unnecessary objects.
The compiled patterns should be pre-built once in the constructor:
// In constructor, after setting this.options:
this.compiledIgnorePatterns = this.options.ignorePaths.map((ignorePath) => {
if (ignorePath.includes("*")) {
const escaped = ignorePath
.split("*")
.map((s) => this.escapeRegExp(s))
.join(".*")
try { return { type: "regex" as const, pattern: new RegExp(`^${escaped}$`) } }
catch { return null }
}
return { type: "exact" as const, value: ignorePath }
}).filter(Boolean)Then shouldIgnorePath just iterates this.compiledIgnorePatterns without creating any objects.
| argv: Record<string, unknown> | ||
| ) => unknown | ||
| loaded = configFactory({}, {}) | ||
| } |
There was a problem hiding this comment.
Async config factory returns a Promise, not the config
If a webpack config exports an async factory (valid in webpack 5), this silently diffs a Promise object instead of the actual config:
// User's config file
module.exports = async (env, argv) => ({ mode: 'production', ... })In that case configFactory({}, {}) returns a Promise, loaded becomes the Promise, and the diff output will be meaningless with no useful error.
Consider adding a check:
if (loaded instanceof Promise) {
throw new Error(
"Async config factories are not supported. The config file returned a Promise."
)
}
| } | ||
|
|
||
| return result.summary.totalChanges > 0 ? 1 : 0 | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
Exit code 1 is used for both "differences found" and runtime errors
The docs acknowledge this, but it makes the tool hard to use in CI:
bin/diff-bundler-config --left=a.yaml --right=b.yaml
echo $? # Is this 1 because files differ, or because --left file wasn't found?A common convention (used by diff, git diff, rubocop --fail-level) is:
0= success, no changes1= differences found (expected, not an error)2= runtime error (file not found, parse error, etc.)
This would let CI scripts distinguish real failures from expected diffs:
bin/diff-bundler-config --left=a.yaml --right=b.yaml
code=$?
if [ $code -eq 2 ]; then echo "Tool error!"; exit 1; fi
| export { getDocForKey, getDocDescription } from "../configDocs"; | ||
| export { ConfigFileLoader, generateSampleConfigFile } from "./configFile"; | ||
| export { BuildValidator } from "./buildValidator"; | ||
| //# sourceMappingURL=index.d.ts.map No newline at end of file |
There was a problem hiding this comment.
Committed build artifact
This file ends with //# sourceMappingURL=index.d.ts.map — a marker emitted by the TypeScript compiler. It's a generated declaration file and shouldn't be committed to source; it should be produced by the build step (e.g., tsc) and listed in .gitignore.
Committing generated .d.ts files can cause stale/conflicting declarations when the source changes, and the missing trailing newline also violates the project's linting rules.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 211443bcb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export { YamlSerializer } from "./yamlSerializer" | ||
| export { FileWriter } from "./fileWriter" | ||
| export { getDocForKey } from "./configDocs" | ||
| export { getDocForKey, getDocDescription } from "../configDocs" |
There was a problem hiding this comment.
Re-exporting getDocForKey from ../configDocs changes the public shakapacker/configExporter API from returning a documentation string (from the previous configExporter/configDocs.ts) to returning a ConfigDoc object; existing consumers that treat this as text (for example string concatenation or rendering) will now output [object Object] or fail at runtime. Keeping the old string contract (or introducing a new object-returning name) avoids a breaking behavior change in a non-major feature commit.
Useful? React with 👍 / 👎.
|
Closing as superseded by #973 ( |
## Summary This PR supersedes [#961](#961) by using the extracted [`pack-config-diff`](https://github.com/shakacode/pack-config-diff) package instead of introducing an in-repo `configDiffer` module. ## What changed - Added `bin/diff-bundler-config` and `lib/install/bin/diff-bundler-config` wrappers. - Added `pack-config-diff` dependency in `package.json`. - Added new guide: `docs/config-diff.md`. - Updated docs/examples to use semantic diffing: - `README.md` (Debugging Configuration section) - `docs/troubleshooting.md` - `docs/rspack_migration_guide.md` - Updated doctor binstub checks/specs to include `bin/diff-bundler-config`. ## Behavior notes - The binstub resolves `pack-config-diff` via shakapacker's dependency tree (using `createRequire`) so strict package managers (pnpm, Yarn PnP) can find it. - If the module cannot be loaded, the binstub prints a diagnostic message and exits with code 2. ## Why this supersedes #961 - #961 introduced and maintained a large internal diff engine under Shakapacker. - This PR keeps Shakapacker focused and delegates diffing to the dedicated extracted project. - Future diff improvements now land in one place (`pack-config-diff`) and are consumed here via the wrapper. ## Validation - `bundle exec rspec spec/shakapacker/doctor_spec.rb` - `bundle exec rspec spec/shakapacker/engine_rake_tasks_spec.rb` - `node bin/diff-bundler-config --help` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds a new shipped CLI wrapper and a new runtime dependency (`pack-config-diff`), plus updates CI/test harness to install via `yalc add`; risk is mainly around packaging/installation and exit-code behavior in developer/CI workflows. > > **Overview** > Introduces a new `bin/diff-bundler-config` (and installer template in `lib/install/bin/`) that wraps `pack-config-diff` to provide *semantic* webpack/rspack configuration diffs with normalized exit codes. > > Updates docs to recommend `bin/diff-bundler-config` instead of raw `diff`, adds a dedicated `docs/config-diff.md` guide, and adjusts `shakapacker:doctor` to treat `diff-bundler-config` as an optional binstub (with new specs to keep shared binstubs in sync). > > CI and rake test tasks switch local package consumption from `yalc link`/`npm install` to `yalc add` with `yarn install`, and the JS tooling config (`knip`) is updated to account for the new binstubs/dependency. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit eef1c25. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Adds a CLI to produce semantic configuration diffs for bundler configs with clear exit codes. * **Documentation** * New Configuration Diff Guide and README troubleshooting subsection; migration guide updated to recommend semantic diffs and show examples. * **Chores / Tests** * Installer/doctor and tests updated to treat the diff CLI as optional. * **CI / Tooling** * Updated local package handling in CI and test tasks; formatting ignore list extended. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
* origin/main: (22 commits) docs: add Dependabot configuration guide (#1094) Sync address-review prompt with upstream PR #16 (#1098) Supersede #910: entry shape test with lint unblock (#919) fix: align rspack v2 peer deps and installer defaults (#1091) docs: update README and guides for Shakapacker v10 (#1092) Release 10.0.0 Update CHANGELOG.md for v10.0.0 (#1089) Release 10.0.0-rc.1 Update CHANGELOG.md for v10.0.0-rc.1 (#1087) Supersede #961 by using pack-config-diff (#973) Add final summary output to rake release (#1041) Add bin/setup to install development deps (#1039) Release 10.0.0-rc.0 Use npx release-it to avoid mise shim failures (#1040) Fix Nokogiri build failure on Ruby 3.4.6 (#1038) Update CHANGELOG.md for v10.0.0-rc.0 (#1037) Update rspack dev deps to 2.0.0-rc.0 (#1036) Fix stale and broken documentation across Shakapacker guides (#1023) Allow webpack-cli v7 in peer dependencies (#1021) refactor: simplify resolving js peer versions when installing (#1034) ... # Conflicts: # package.json
* origin/main: docs: add Dependabot configuration guide (#1094) Sync address-review prompt with upstream PR #16 (#1098) Supersede #910: entry shape test with lint unblock (#919) fix: align rspack v2 peer deps and installer defaults (#1091) docs: update README and guides for Shakapacker v10 (#1092) Release 10.0.0 Update CHANGELOG.md for v10.0.0 (#1089) Release 10.0.0-rc.1 Update CHANGELOG.md for v10.0.0-rc.1 (#1087) Supersede #961 by using pack-config-diff (#973) # Conflicts: # CHANGELOG.md # Rakefile
2 participants
Summary
configDiffermodule for comparing webpack/rspack configuration filesbin/diff-bundler-config) for comparing exported YAML/JSON/JS/TS configsWhat's included
--left/--rightfile comparisonTest plan
yarn lint)Closes #667
🤖 Generated with Claude Code
Note
Medium Risk
Adds a new CLI that dynamically loads and executes
.js/.tsconfig files (including optionalts-node), so regressions or unexpected execution behavior could affect developer workflows despite being opt-in and not production-path.Overview
Adds a new
configDifferpackage (exported asshakapacker/configDiffer) plus abin/diff-bundler-configCLI to compare two webpack/rspack config files (YAML/JSON/JS/TS) with optional path normalization, ignored keys/paths, depth limiting, and multiple output formats (contextual detailed, summary, JSON, YAML).Updates docs (
README.md, newdocs/config-diff.md) to document exporting+diffing workflows, adds installer binstub (lib/install/bin/diff-bundler-config), introduces targeted ESLint overrides for the new TypeScript module, and adds unit tests and example YAML configs plus ascripts/ci-local.shhelper for running CI checks locally.Written by Cursor Bugbot for commit b0f556e. Configure here.
Summary by CodeRabbit
Release Notes
New Features
diff-bundler-configCLI command to compare webpack/rspack configurations with support for JSON, YAML, and human-readable output formats.Documentation
Tests
Chores