Conversation
Co-authored-by: yamyam263 <yamyam263@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughEnhanced size-limit runner: when webpack isn't enabled, derive an enhanced config forcing esbuild and marking Node.js built-ins as external/ignored; replace the temp package.json size-limit entry with this config; run size-limit while attempting JSON parse of stdout/stderr even on non-zero exit, fallback to text parsing, log results and return any found results, preserving cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Action
participant CLI as size-limit CLI
participant Parser as Output Parser
participant Logger
rect rgba(220,240,200,0.20)
Note over Action,CLI: Build enhanced config (esbuild, builtins externals) and replace temp package.json entry
Action->>CLI: spawn size-limit with temp package.json using enhanced config
CLI-->>Action: exit code (0 or non-zero) + output (stdout/stderr)
end
rect rgba(200,220,240,0.16)
Note over Action,Parser: Parse outputs regardless of exit code
Action->>Parser: try parse JSON from stdout and stderr
alt JSON parsed
Parser-->>Action: results[]
Action->>Logger: log per-result details (even if exit ≠ 0)
Action-->>Action: return results
else JSON fail
Action->>Parser: fallback to text parsing
Parser-->>Action: results[] or []
Action->>Logger: log outcome / errors
Action-->>Action: return results or []
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1).github/actions/size-limit/src/size-limit/package.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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 |
github-actions
bot
added
the
github actions
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
yamcodes
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/size-limit/src/size-limit/package.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/actions/size-limit/src/size-limit/package.ts (2)
.github/actions/size-limit/src/types.ts (1)
SizeLimitResult(1-8).github/actions/size-limit/src/utils/parser.ts (1)
parseSizeLimitOutput(6-233)
🔇 Additional comments (3)
.github/actions/size-limit/src/size-limit/package.ts (3)
122-122: LGTM!Correctly uses the enhanced configuration with Node.js externals.
197-248: Excellent error handling for non-zero exit codes!The implementation correctly addresses the PR objective by extracting size data even when size-limit fails. The flow is logical:
- Parse JSON output regardless of exit code
- Fallback to text parsing if JSON fails
- Return results when available, even with non-zero exit
- Log detailed information for debugging
The concatenation of
stdout + stderron line 221 for text parsing is a reasonable fallback, though the order of interleaved output might vary.
51-93: I need the actual complete module lists from Node.js v22 and v23 to compare. Let me search for these:The review comment is based on a misunderstanding of Node.js module architecture.
The official Node.js documentation notes that the
module.builtinModuleslist doesn't contain prefix-only modules likenode:test, and this is by design. Thenode:sqlitemodule must be imported using the node: prefix, andnode:seais accessed viarequire('node:sea')for Single Executable Applications.Some built-in modules must be requested with the
node:prefix to prevent newly introduced built-in modules from conflicting with userland packages that already took the name. The list in the code (lines 51-93) contains modules that can be required without the prefix, which semantically aligns with howmodule.builtinModulesworks.The modules you mentioned (
node:test,node:sqlite,node:sea) are deliberately excluded from such lists because they require the prefix and are never accessible without it. Including them would be architecturally incorrect.Likely an incorrect or invalid review comment.
![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.
ℹ️ 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".
| // Enhance size-limit config to mark Node.js built-in modules as external | ||
| // This prevents bundling errors when size-limit tries to bundle Node.js built-in modules | ||
| const enhancedSizeLimitConfig = sizeLimitConfig.map((config) => { | ||
| // If webpack is already configured, preserve it | ||
| if (config.webpack !== undefined) { | ||
| return config; | ||
| } | ||
|
|
||
| // For Node.js packages, configure esbuild to mark Node.js built-in modules as external | ||
| // This prevents esbuild from trying to bundle node:path, node:fs, etc. | ||
| // size-limit uses esbuild when webpack: false, and we can configure externals | ||
| const nodeBuiltinModules = [ | ||
| "assert", | ||
| "async_hooks", | ||
| "buffer", | ||
| "child_process", | ||
| "cluster", | ||
| "console", | ||
| "constants", | ||
| "crypto", | ||
| "dgram", | ||
| "diagnostics_channel", | ||
| "dns", | ||
| "domain", | ||
| "events", | ||
| "fs", | ||
| "http", | ||
| "http2", | ||
| "https", | ||
| "inspector", | ||
| "module", | ||
| "net", | ||
| "os", | ||
| "path", | ||
| "perf_hooks", | ||
| "process", | ||
| "punycode", | ||
| "querystring", | ||
| "readline", | ||
| "repl", | ||
| "stream", | ||
| "string_decoder", | ||
| "sys", | ||
| "timers", | ||
| "tls", | ||
| "trace_events", | ||
| "tty", | ||
| "url", | ||
| "util", | ||
| "v8", | ||
| "vm", | ||
| "worker_threads", | ||
| "zlib", | ||
| ]; | ||
|
|
||
| return { | ||
| ...config, | ||
| webpack: false, // Use esbuild instead of webpack | ||
| // Configure esbuild to mark Node.js built-in modules as external | ||
| // This tells esbuild not to try to bundle these modules | ||
| // size-limit passes this to esbuild's external option | ||
| ignore: [ | ||
| ...(Array.isArray(config.ignore) ? config.ignore : []), | ||
| ...nodeBuiltinModules.map((m) => `node:${m}`), | ||
| ...nodeBuiltinModules, | ||
| ], |
There was a problem hiding this comment.
The new mapping only keeps config.ignore when it is already an array. size-limit accepts ignore as a string or RegExp as well, and several configs in the ecosystem rely on that shorthand. With the current logic any non‑array value is dropped entirely, so previously ignored packages will suddenly be bundled and counted in the size calculation, defeating the original configuration. Normalise single values (e.g. wrap in an array) rather than clearing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@yamcodes No, this issue is not solved in the current code.
Looking at lines 102-106 of the file, the code currently does:
ignore: [
...(Array.isArray(config.ignore) ? config.ignore : []),
...nodeBuiltinModules.map((m) => `node:${m}`),
...nodeBuiltinModules,
],This drops any non-array ignore values (strings, RegExp, etc.). The fix should normalize single values by wrapping them in an array:
ignore: [
...(config.ignore
? (Array.isArray(config.ignore) ? config.ignore : [config.ignore])
: []
),
...nodeBuiltinModules.map((m) => `node:${m}`),
...nodeBuiltinModules,
],This ensures that:
- If
config.ignoreis an array → use it as-is - If
config.ignoreis a string or RegExp → wrap it in an array - If
config.ignoreis undefined → use an empty array
This preserves the original ignore configuration regardless of its type.
| // Enhance size-limit config to mark Node.js built-in modules as external | ||
| // This prevents bundling errors when size-limit tries to bundle Node.js built-in modules | ||
| const enhancedSizeLimitConfig = sizeLimitConfig.map((config) => { | ||
| // If webpack is already configured, preserve it | ||
| if (config.webpack !== undefined) { | ||
| return config; | ||
| } | ||
|
|
||
| // For Node.js packages, configure esbuild to mark Node.js built-in modules as external | ||
| // This prevents esbuild from trying to bundle node:path, node:fs, etc. | ||
| // size-limit uses esbuild when webpack: false, and we can configure externals | ||
| const nodeBuiltinModules = [ | ||
| "assert", | ||
| "async_hooks", | ||
| "buffer", | ||
| "child_process", | ||
| "cluster", | ||
| "console", | ||
| "constants", | ||
| "crypto", | ||
| "dgram", | ||
| "diagnostics_channel", | ||
| "dns", | ||
| "domain", | ||
| "events", | ||
| "fs", | ||
| "http", | ||
| "http2", | ||
| "https", | ||
| "inspector", | ||
| "module", | ||
| "net", | ||
| "os", | ||
| "path", | ||
| "perf_hooks", | ||
| "process", | ||
| "punycode", | ||
| "querystring", | ||
| "readline", | ||
| "repl", | ||
| "stream", | ||
| "string_decoder", | ||
| "sys", | ||
| "timers", | ||
| "tls", | ||
| "trace_events", | ||
| "tty", | ||
| "url", | ||
| "util", | ||
| "v8", | ||
| "vm", | ||
| "worker_threads", | ||
| "zlib", | ||
| ]; | ||
|
|
||
| return { | ||
| ...config, | ||
| webpack: false, // Use esbuild instead of webpack | ||
| // Configure esbuild to mark Node.js built-in modules as external | ||
| // This tells esbuild not to try to bundle these modules | ||
| // size-limit passes this to esbuild's external option | ||
| ignore: [ | ||
| ...(Array.isArray(config.ignore) ? config.ignore : []), | ||
| ...nodeBuiltinModules.map((m) => `node:${m}`), | ||
| ...nodeBuiltinModules, | ||
| ], | ||
| }; | ||
| }); |
There was a problem hiding this comment.
When no webpack property is provided, the code now forces webpack: false, which means size-limit will run with the esbuild plugin even for packages whose default preset is webpack (e.g. those depending on @size-limit/preset-small-lib). That change drops webpack-specific behaviour such as Babel loaders or aliases and can produce different bundle contents or outright bundling failures unrelated to the original issue. If the goal is only to externalise Node built-ins, consider extending the existing configuration without changing the bundler, or restrict the change to configs that were already using esbuild.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@yamcodes No, the concern raised by the bot is not fully solved.
Looking at the current code (lines 42-44 in the file), the condition checks:
if (config.webpack === true) {
return config;
}This only preserves the original config when webpack is explicitly set to true. When webpack is undefined (not specified), the code proceeds to force webpack: false, which changes the default bundler behavior for packages that rely on preset defaults (like @size-limit/preset-small-lib which uses webpack by default).
The issue: This unconditionally switches all configs without an explicit webpack: true to use esbuild, potentially breaking packages that depend on webpack-specific features.
Suggested fix: Change line 42 to preserve any existing webpack configuration:
// If webpack is already configured (true or false), preserve it
if (config.webpack !== undefined) {
return config;
}This way:
- Configs with
webpack: true→ preserved (uses webpack) - Configs with
webpack: false→ preserved (already using esbuild) - Configs with
webpack: undefined→ preserved (uses preset default, typically webpack)
Only when webpack is truly undefined would the enhancement apply, but we should reconsider whether forcing esbuild in that case is correct. The safer approach might be to only add the Node.js built-ins to the ignore array without changing the webpack setting at all.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Fixes bundling with undefined ignore - Add node: prefix to node modules
2 participants
Extract size data from failed size-limit checks and mark Node.js built-in modules as external to fix '—' output and bundling errors.
The "—" output occurred because the action would exit early if
size-limitfailed, preventing the extraction of size data needed for comparison. By parsing the JSON output even on failure, we can still get the size information. The bundling errors were due tosize-limit(using esbuild) attempting to bundle Node.js core modules, which are external to browser/frontend bundles. Explicitly marking them asexternalresolves this.Closes #362
Summary by CodeRabbit
Bug Fixes
Improvements