-
-
Notifications
You must be signed in to change notification settings - Fork 199
fix(theme/eject): allow treeshaking manually the theme/index file #2895
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: main
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for rspress-v2 failed. Why did it fail? →
|
Rsdoctor Bundle Diff Analysis📁 webPath:
📦 Download Diff Report: web Bundle Diff Generated by Rsdoctor GitHub Action |
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.
Pull request overview
This PR introduces an export star optimizer that transforms export * statements into explicit named exports to enable better tree-shaking. The optimizer uses regex-based parsing to identify all exports from referenced modules and replaces wildcard exports with explicit export lists, excluding locally-defined exports to avoid conflicts.
- Implements an AST-like transformation using regex patterns to parse and transform export statements
- Adds a webpack/rspack loader that applies the optimization to theme index files during build
- Includes test coverage using memfs for file system mocking
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/node/theme/loader.ts | Implements the rspack loader that applies the export star optimization transform |
| packages/core/src/node/theme/exportStarOptimizerTransform.ts | Core transformation logic that parses exports and replaces export * with named exports |
| packages/core/src/node/theme/exportStarOptimizerTransform.test.ts | Test suite covering basic transformation scenarios using memfs |
| packages/core/src/node/initRsbuild.ts | Integrates the loader into the build pipeline for theme index files |
| packages/core/rslib.config.ts | Applies the optimizer during library build and adds loader entry point |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sideEffects: false, | ||
| }); | ||
| themeIndexRule | ||
| .use('EXPORT_STAR_OPTIMIZE') |
Copilot
AI
Dec 15, 2025
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.
The loader use name 'EXPORT_STAR_OPTIMIZE' is in SCREAMING_SNAKE_CASE while other loaders in the codebase typically use descriptive strings in kebab-case or regular naming. Consider using a more conventional name like 'export-star-optimizer' for consistency.
| .use('EXPORT_STAR_OPTIMIZE') | |
| .use('export-star-optimizer') |
| const varExportRegex = /export\s+(?:const|let|var)\s+(\w+)/g; | ||
| let match: RegExpExecArray | null; | ||
| while ((match = varExportRegex.exec(cleanCode)) !== null) { | ||
| exports.add(match[1]); |
Copilot
AI
Dec 15, 2025
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.
The same regex bug exists in getModuleExports: /export\s+(?:const|let|var)\s+(\w+)/g only captures the first variable in multi-variable declarations like 'export const a = 1, b = 2;'. This inconsistency with parseLocalExports should be fixed to handle comma-separated declarations.
| const varExportRegex = /export\s+(?:const|let|var)\s+(\w+)/g; | |
| let match: RegExpExecArray | null; | |
| while ((match = varExportRegex.exec(cleanCode)) !== null) { | |
| exports.add(match[1]); | |
| const varExportRegex = /export\s+(?:const|let|var)\s+([^;]+)/g; | |
| let match: RegExpExecArray | null; | |
| while ((match = varExportRegex.exec(cleanCode)) !== null) { | |
| // match[1] contains the full variable declaration list, e.g. "a = 1, b = 2" | |
| const varList = match[1]; | |
| // Split by commas not inside brackets (to avoid destructuring confusion) | |
| varList.split(',').forEach(decl => { | |
| // Remove any default value assignment and destructuring | |
| // Only match simple variable names (skip destructuring for now) | |
| const nameMatch = /^\s*([a-zA-Z_$][\w$]*)/.exec(decl.trim()); | |
| if (nameMatch) { | |
| exports.add(nameMatch[1]); | |
| } | |
| }); |
| // Handle `name as alias`. | ||
| const parts = n.trim().split(/\s+as\s+/); | ||
| return parts[parts.length - 1].trim(); | ||
| }); |
Copilot
AI
Dec 15, 2025
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.
The parsing logic for 'export { name as alias }' is incorrect. When splitting by 'as', parts[parts.length - 1] gets the alias (exported name), but for local exports in the parseLocalExports method, we should track the alias, not the original name. However, this logic seems backwards - it should store the exported name (after 'as'), which is correct. But the logic for adding names from lines 49 doesn't properly validate empty strings after trim(), which could add empty entries to localExports if there are trailing commas or extra spaces.
| }); | |
| }).filter(name => name.length > 0); |
| while ((match = exportStarRegex.exec(code)) !== null) { | ||
| const fullMatch = match[0]; | ||
| const source = match[1]; | ||
| const startIndex = match.index; | ||
| const endIndex = startIndex + fullMatch.length; | ||
|
|
||
| // Get exports of the target module. | ||
| const moduleExports = await this.getModuleExports(source); |
Copilot
AI
Dec 15, 2025
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.
Each 'export *' statement triggers an async module resolution and file read operation in getModuleExports. If a file has multiple 'export *' statements referencing the same module, this module will be resolved and read multiple times. Consider caching the results of getModuleExports by module path to avoid redundant I/O operations.
| while ((match = exportStarRegex.exec(code)) !== null) { | |
| const fullMatch = match[0]; | |
| const source = match[1]; | |
| const startIndex = match.index; | |
| const endIndex = startIndex + fullMatch.length; | |
| // Get exports of the target module. | |
| const moduleExports = await this.getModuleExports(source); | |
| // Cache to avoid redundant module resolution and file reads. | |
| const moduleExportsCache = new Map<string, Set<string>>(); | |
| while ((match = exportStarRegex.exec(code)) !== null) { | |
| const fullMatch = match[0]; | |
| const source = match[1]; | |
| const startIndex = match.index; | |
| const endIndex = startIndex + fullMatch.length; | |
| // Get exports of the target module, using cache if available. | |
| let moduleExports: Set<string>; | |
| if (moduleExportsCache.has(source)) { | |
| moduleExports = moduleExportsCache.get(source)!; | |
| } else { | |
| moduleExports = await this.getModuleExports(source); | |
| moduleExportsCache.set(source, moduleExports); | |
| } |
| * Parse source code to collect local exports. | ||
| */ | ||
| parseLocalExports(code: string): void { | ||
| // 移除注释 |
Copilot
AI
Dec 15, 2025
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.
Chinese comment in code. Consider translating to English for consistency with the rest of the codebase. The comment reads "移除注释" which means "Remove comments".
| // 移除注释 | |
| // Remove comments |
| const resolver = new rspack.experiments.resolver.ResolverFactory({ | ||
| extensions: ['.ts', '.tsx', '.mjs', '.js', '.jsx', '.json'], | ||
| mainFiles: ['index'], | ||
| mainFields: ['module', 'browser', 'main'], | ||
| alias: {}, | ||
| }); |
Copilot
AI
Dec 15, 2025
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.
A new ResolverFactory instance is created on every call to getModuleExports. This is inefficient, especially when processing multiple 'export *' statements. Consider creating the resolver once (e.g., in the constructor or as a class property) and reusing it across all resolutions, similar to the pattern used in flattenMdxContent.ts and reactAlias.ts in this codebase.
| */ | ||
| removeComments(code: string): string { | ||
| // 移除单行注释 | ||
| code = code.replace(/\/\/.*$/gm, ''); |
Copilot
AI
Dec 15, 2025
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.
The regex for removing single-line comments will incorrectly remove URLs and strings that contain '//', such as 'https://example.com' or strings like 'const url = "http://foo"'. This could corrupt valid code. Consider using a more robust comment removal approach that respects string literals and template literals, or use a proper AST-based parser.
| setup(api) { | ||
| api.transform( | ||
| { test: /src\/theme\/index\.ts/ }, | ||
| ({ code, resourcePath }) => { |
Copilot
AI
Dec 15, 2025
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.
The transform function is defined inline as an async function but is not explicitly marked as async in the api.transform call. This could lead to issues if the transform is expected to return a Promise. Ensure the transform callback properly handles the async nature of exportStarOptimizerTransform by either using async/await or returning the Promise.
| ({ code, resourcePath }) => { | |
| async ({ code, resourcePath }) => { |
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.warn(`无法解析模块 ${modulePath}: ${message}`); | ||
| return new Set(); | ||
| } |
Copilot
AI
Dec 15, 2025
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.
When module resolution fails, the method returns an empty Set and only logs a warning. This silently causes the 'export *' statement to be replaced with nothing, which changes the semantic behavior of the code. If a module genuinely exists but fails to resolve, this could break the application. Consider either preserving the original 'export *' statement on resolution failure, or making the error more visible (e.g., throwing an error in non-production builds).
| removeComments(code: string): string { | ||
| // 移除单行注释 | ||
| code = code.replace(/\/\/.*$/gm, ''); | ||
| // 移除多行注释 |
Copilot
AI
Dec 15, 2025
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.
Chinese comment in code. Consider translating to English for consistency. The comment reads "移除多行注释" which means "Remove multi-line comments".
| // 移除多行注释 | |
| // Remove multi-line comments |
2c4dbb6 to
8b7a410
Compare
Summary
If we craft a manual
export *treeshaking optimizer, it can tackle both the bundle size problem and the prior issue where CSS couldn't be treeshaken effectively.// before
// after
Got
export *to export{ Search, Button, Banner }, optimized to the following fileRelated Issue
Checklist