-
Notifications
You must be signed in to change notification settings - Fork 116
chore: migrate from webpack to rsbuild #1762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Replace webpack with @rsbuild/core for main extension build - Create rsbuild.config.ts with TypeScript decorator support via ts-loader - Update package.json scripts (webpack → build, watch mode updated) - Remove webpack dependencies: webpack, webpack-cli, copy-webpack-plugin, terser-webpack-plugin, webpack-shell-plugin-next - Update .vscode/launch.json and tasks.json for rsbuild compatibility - Maintain webview panels build with Vite (no changes) - Configure proper externals, source maps, and file copying for Python scripts - Production build size: ~2.1 MB (optimized) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis pull request migrates the build system from webpack to rsbuild across the extension project. Changes include updating npm scripts to use rsbuild commands, replacing webpack dependencies with @rsbuild/core, updating VS Code launch and task configurations to reference the new build system, creating a comprehensive rsbuild configuration file, and removing the webpack configuration file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve a cohesive build system migration across multiple configuration files. While the rsbuild configuration is comprehensive and requires careful verification, the changes follow a clear migration pattern rather than introducing heterogeneous logic. Review focus should address rsbuild configuration correctness, externals handling, asset copying logic, and verification that npm scripts properly align with the new build tooling. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 9632e09 in 1 minute and 32 seconds. Click for details.
- Reviewed
442lines of code in3files - Skipped
3files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. rsbuild.config.ts:96
- Draft comment:
Consider adding an explicit type annotation for the 'api' parameter in the plugin's setup callback to improve clarity and maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% One of the additional rules emphasizes providing return types for utility functions. While this isn't exactly about return types, type annotations in TypeScript generally improve code clarity and maintainability. However, suggestinganyas a type is actually an anti-pattern in TypeScript as it defeats the purpose of type safety. Without knowing the actual type from the rsbuild API, suggestinganycould be worse than letting TypeScript infer the type. The comment suggests usinganywhich is generally discouraged in TypeScript. Without access to rsbuild's type definitions, we can't be certain of the correct type to use. While type annotations are generally good, suggesting an incorrect or overly permissive type likeanycould be more harmful than helpful. The comment should be deleted since it suggests usinganytype which is an anti-pattern, and we don't have enough context to suggest the correct type.
Workflow ID: wflow_7fTSEYcqlGvMs1bA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| extensions: [".ts", ".js"], | ||
| }, | ||
| tools: { | ||
| rspack: (config) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an explicit return type for the rspack callback function. For instance, annotate the parameter and return type (e.g., (config: any): any => { ... }) to improve type safety and ease future refactoring.
| rspack: (config) => { | |
| rspack: (config: any): any => { |
This comment was generated because it violated a code review rule: mrule_lKhrJQCKUgYAxwcS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
package.json (1)
1366-1366: Remove leftover webpack-era dependency.file-loader is unused with Rsbuild; safe to drop.
- "file-loader": "^6.1.0",rsbuild.config.ts (4)
53-57: Prefer output.library.type over deprecated libraryTarget.Rspack/Webpack 5 style is library: { type: "commonjs2" }.
- config.output = { - ...config.output, - libraryTarget: "commonjs2", - devtoolModuleFilenameTemplate: "../[resource-path]", - }; + config.output = { + ...config.output, + library: { type: "commonjs2" }, + devtoolModuleFilenameTemplate: "../[resource-path]", + };
63-71: Rule filter is brittle; may remove unrelated rules.Filtering any rule whose test contains “ts” can also drop combined JS/TS rules. Narrow the predicate to only ts/tsx.
- config.module.rules = config.module.rules.filter((rule: any) => { - if (typeof rule === "object" && rule.test) { - const testStr = rule.test.toString(); - return !testStr.includes("ts"); - } - return true; - }); + const isTsTest = (test: unknown) => + test instanceof RegExp && /\\.[cm]?tsx?$/.test(test.source || ""); + config.module.rules = config.module.rules.filter((rule: any) => { + return !(rule && rule.test && isTsTest(rule.test)); + });
73-81: ts-loader options for decorators and speed.If you rely on legacy decorators/metadata (Inversify), ensure tsconfig has experimentalDecorators and emitDecoratorMetadata. Consider transpileOnly:true + separate type-check for faster watch.
config.module.rules.push({ test: /\.ts$/, exclude: /(node_modules|src\/test)/, use: [ { loader: "ts-loader", + options: { + transpileOnly: true + }, }, ], });Confirm fork-ts-checker is not required in your flow; otherwise keep full type-check in loader.
141-157: Make module copy paths robust across package managers.Hardcoding ./node_modules may break with workspaces/PnP. Derive paths via require.resolve.
- const notebookModules = [ - "./node_modules/zeromq", - "./node_modules/@aminya/node-gyp-build", - "./node_modules/node-gyp-build", - ]; + const notebookModules = [ + path.dirname(require.resolve("zeromq/package.json")), + path.dirname(require.resolve("@aminya/node-gyp-build/package.json")), + path.dirname(require.resolve("node-gyp-build/package.json")), + ];Also compute destPath using path.join("dist","node_modules",pkgName) rather than string replace to avoid OS-specific issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.vscode/launch.json(1 hunks).vscode/tasks.json(1 hunks)package.json(3 hunks)rsbuild.config.ts(1 hunks)webpack.config.js(0 hunks)
💤 Files with no reviewable changes (1)
- webpack.config.js
⏰ 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). (6)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
.vscode/launch.json (1)
15-15: Switch to “npm: build” looks correct.Matches the new build script and dist path. No further changes needed here.
package.json (2)
1324-1324: Double-check prepublish pipeline still valid.You swapped webpack for rsbuild, but still invoke prepareBuild.js. Ensure it doesn’t rely on webpack artifacts.
If needed, I can adapt prepareBuild.js to the new Rsbuild outputs.
1345-1345: Node version requirement for Rsbuild 1.5.x.Ensure CI/dev use Node >= 18.12.0, otherwise builds may fail. Based on learnings.
If your workflows pin an older Node, update them accordingly.
rsbuild.config.ts (1)
20-22: Validate sourceMap setting.If output.sourceMap.js expects boolean or "source-map" is unsupported in your Rsbuild version, switch to true or the documented enum. Based on learnings.
Run rsbuild inspect to confirm the generated devtool/source map settings.
.vscode/tasks.json (1)
23-24: Review comment is incorrect—current patterns already match Rsbuild's actual output.The Rsbuild CLI prints "build started" using logger.start, which is lowercase. The current patterns in
.vscode/tasks.jsonare already correct:
"beginsPattern": "^.*build started.*"matches Rsbuild's actual output- The suggestion to make patterns case-insensitive (
[Bb]uild) is unnecessaryThe review comment incorrectly claims Rsbuild outputs capitalized "Build started" and "Built in …", but the evidence shows Rsbuild emits lowercase "build started". No changes are required.
Likely an incorrect or invalid review comment.
| "watch": "concurrently \"npm:watch --prefix ./webview_panels\" \"rsbuild build --mode development --watch\"", | ||
| "test-compile": "tsc -p ./", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken watch command: “npm:watch” is not a shell command.
concurrently executes shell commands; the “npm:” prefix is a VS Code tasks label, not executable. Watch will fail, breaking debug-task.
Apply this fix:
- "watch": "concurrently \"npm:watch --prefix ./webview_panels\" \"rsbuild build --mode development --watch\"",
+ "watch": "concurrently \"npm run watch --prefix ./webview_panels\" \"rsbuild build --mode development --watch\"",🤖 Prompt for AI Agents
In package.json around lines 1326-1327, the "watch" script uses the invalid
command "npm:watch" (a VS Code task label) which fails when run by concurrently;
change it to call the npm script runner, e.g. replace "npm:watch --prefix
./webview_panels" with "npm run watch --prefix ./webview_panels" (or
alternatively "cd ./webview_panels && npm run watch") so concurrently executes a
real shell command and the debug/watch task works.
| config.externals = [ | ||
| "vscode", | ||
| "commonjs", | ||
| // These dependencies are ignored because we don't use them, and App Insights has try-catch protecting their loading if they don't exist | ||
| // See: https://github.com/microsoft/vscode-extension-telemetry/issues/41#issuecomment-598852991 | ||
| "applicationinsights-native-metrics", | ||
| "@opentelemetry/tracing", | ||
| "@azure/opentelemetry-instrumentation-azure-sdk", | ||
| "@opentelemetry/instrumentation", | ||
| "@azure/functions-core", | ||
| "zeromq", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect externals: literal "commonjs" included; externals type not set.
Including "commonjs" in the array externalizes a non-existent module. Set externalsType and remove it to ensure vscode/zeromq resolve correctly at runtime.
- config.externals = [
- "vscode",
- "commonjs",
- // These dependencies are ignored because we don't use them, and App Insights has try-catch protecting their loading if they don't exist
- // See: https://github.com/microsoft/vscode-extension-telemetry/issues/41#issuecomment-598852991
- "applicationinsights-native-metrics",
- "@opentelemetry/tracing",
- "@azure/opentelemetry-instrumentation-azure-sdk",
- "@opentelemetry/instrumentation",
- "@azure/functions-core",
- "zeromq",
- ];
+ // Ensure CommonJS externalization for Node/VSC extension host
+ config.externalsType = "commonjs";
+ config.externals = [
+ "vscode",
+ "applicationinsights-native-metrics",
+ "@opentelemetry/tracing",
+ "@azure/opentelemetry-instrumentation-azure-sdk",
+ "@opentelemetry/instrumentation",
+ "@azure/functions-core",
+ "zeromq",
+ ];📝 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.
| config.externals = [ | |
| "vscode", | |
| "commonjs", | |
| // These dependencies are ignored because we don't use them, and App Insights has try-catch protecting their loading if they don't exist | |
| // See: https://github.com/microsoft/vscode-extension-telemetry/issues/41#issuecomment-598852991 | |
| "applicationinsights-native-metrics", | |
| "@opentelemetry/tracing", | |
| "@azure/opentelemetry-instrumentation-azure-sdk", | |
| "@opentelemetry/instrumentation", | |
| "@azure/functions-core", | |
| "zeromq", | |
| ]; | |
| // Ensure CommonJS externalization for Node/VSC extension host | |
| config.externalsType = "commonjs"; | |
| config.externals = [ | |
| "vscode", | |
| "applicationinsights-native-metrics", | |
| "@opentelemetry/tracing", | |
| "@azure/opentelemetry-instrumentation-azure-sdk", | |
| "@opentelemetry/instrumentation", | |
| "@azure/functions-core", | |
| "zeromq", | |
| ]; |
🤖 Prompt for AI Agents
In rsbuild.config.ts around lines 34 to 45, remove the literal "commonjs" from
the config.externals array and set the externals type explicitly (e.g.,
config.externalsType = "commonjs" or "commonjs2") so external modules like
"vscode" and "zeromq" are resolved correctly at runtime; keep the listed module
names in the externals array but do not include the string "commonjs".
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Overview
Problem
Describe the problem you are solving. Mention the ticket/issue if applicable.
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Migrate build system from Webpack to rsbuild, updating configuration and scripts, and removing Webpack dependencies.
@rsbuild/corefor main extension build.rsbuild.config.tsfor configuration with TypeScript decorator support viats-loader.package.jsonscripts to usersbuild(e.g.,build,watch).webpack,webpack-cli,copy-webpack-plugin,terser-webpack-plugin,webpack-shell-plugin-next.@rsbuild/coretodevDependencies..vscode/launch.jsonandtasks.jsonfor rsbuild compatibility.rsbuild.config.ts.This description was created by
for 9632e09. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit