-
Notifications
You must be signed in to change notification settings - Fork 8
Feat: add basehref #74
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
Conversation
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Created a utility function to standardize base path handling across all plugins. The function handles special cases like root paths, and strips leading/trailing slashes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Implemented a function to transform asset paths by applying baseHref prefix: - Uses normalizeBasePath for consistent formatting - Preserves index.html and absolute paths - Creates deep copies of assets to avoid modifying originals - Includes comprehensive unit tests 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Implemented webpack-basehref-integration.ts utility to extract baseHref from multiple sources - Added priority logic: Plugin options > HTML Plugin > publicPath - Updated ZeWebpackPlugin and ZeRspackPlugin to detect and store baseHref in ZephyrEngine - Modified build-webpack-assets-map to apply baseHref to asset paths 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Extract base property from Vite configuration - Added support for explicit baseHref override in plugin options - Store baseHref in ZephyrEngine.buildProperties - Normalize baseHref using utility functions - Apply baseHref to asset paths with special case handling 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
View your CI Pipeline Execution ↗ for commit 9e5cb18.
☁️ Nx Cloud last updated this comment at |
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.
I see some inconsistency, I would love to make a more in depth code review
Claude finished @fiorin's task —— View job PR Review AnalysisThis PR adds baseHref support to Zephyr bundler plugins, enabling custom public paths for assets across Webpack, Rspack, Rolldown, Rollup, and Vite. Todo List:
Code Review ResultsIssues & Recommendations
Critical Issues Requiring Immediate Attention1. Rolldown Plugin Incorrect Implementation // Current (WRONG):
zephyr_engine.buildProperties.baseHref = _options.dir;
// Should be:
zephyr_engine.buildProperties.baseHref = _options.publicPath;
// OR implement proper detection like other bundlers 2. Incomplete Rsbuild Plugin
// Suggested implementation:
apply(compiler: any): void {
this._options.zephyr_engine.buildProperties.output = compiler.outputPath;
detectAndStoreBaseHref(this._options.zephyr_engine, compiler);
logBuildSteps(this._options, compiler);
setupZeDeploy(this._options, compiler);
} Performance ConsiderationsMemory Optimization Needed: // Current inefficient approach in ze-basehref-handler.ts:111
Object.values(assetsMap).forEach((asset) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
});
// More efficient:
Object.entries(assetsMap).forEach(([key, asset]) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
}); Security Analysis✅ No critical security vulnerabilities found
Test Coverage AssessmentStrong: Core basehref handler has excellent test coverage (ze-basehref-handler.spec.ts)
Code Quality Score: 6/10Strengths:
Areas for Improvement:
Top 3 Next Steps:
|
Claude finished @fiorin's task —— View job PR Review AnalysisThis PR adds baseHref support to Zephyr bundler plugins, enabling custom public paths for assets across Webpack, Rspack, Rolldown, Rollup, and Vite. Todo List:
Code Review ResultsIssues & Recommendations
Critical Issues Requiring Immediate Attention1. Rolldown Plugin Incorrect Implementation // Current (WRONG):
zephyr_engine.buildProperties.baseHref = _options.dir;
// Should be:
zephyr_engine.buildProperties.baseHref = _options.publicPath;
// OR implement proper detection like other bundlers 2. Incomplete Rsbuild Plugin
// Suggested implementation:
apply(compiler: any): void {
this._options.zephyr_engine.buildProperties.output = compiler.outputPath;
detectAndStoreBaseHref(this._options.zephyr_engine, compiler);
logBuildSteps(this._options, compiler);
setupZeDeploy(this._options, compiler);
} Performance ConsiderationsMemory Optimization Needed: // Current inefficient approach in ze-basehref-handler.ts:111
Object.values(assetsMap).forEach((asset) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
});
// More efficient:
Object.entries(assetsMap).forEach(([key, asset]) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
}); Security Analysis✅ No critical security vulnerabilities found
Test Coverage AssessmentStrong: Core basehref handler has excellent test coverage (ze-basehref-handler.spec.ts)
Code Quality Score: 6/10Strengths:
Areas for Improvement:
Top 3 Next Steps:
|
Claude finished @fiorin's task —— View job PR Review AnalysisThis PR adds baseHref support to Zephyr bundler plugins, enabling custom public paths for assets across Webpack, Rspack, Rolldown, Rollup, and Vite. Todo List:
Code Review ResultsIssues & Recommendations
Critical Issues Requiring Immediate Attention1. Rolldown Plugin Incorrect Implementation // Current (WRONG):
zephyr_engine.buildProperties.baseHref = _options.dir;
// Should be:
zephyr_engine.buildProperties.baseHref = _options.publicPath;
// OR implement proper detection like other bundlers 2. Incomplete Rsbuild Plugin
// Suggested implementation:
apply(compiler: any): void {
this._options.zephyr_engine.buildProperties.output = compiler.outputPath;
detectAndStoreBaseHref(this._options.zephyr_engine, compiler);
logBuildSteps(this._options, compiler);
setupZeDeploy(this._options, compiler);
} Performance ConsiderationsMemory Optimization Needed: // Current inefficient approach in ze-basehref-handler.ts:111
Object.values(assetsMap).forEach((asset) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
});
// More efficient:
Object.entries(assetsMap).forEach(([key, asset]) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
}); Security Analysis✅ No critical security vulnerabilities found
Test Coverage AssessmentStrong: Core basehref handler has excellent test coverage (ze-basehref-handler.spec.ts)
Code Quality Score: 6/10Strengths:
Areas for Improvement:
Top 3 Next Steps:
|
Claude finished @fiorin's task —— View job PR Review AnalysisThis PR adds baseHref support to Zephyr bundler plugins, enabling custom public paths for assets across Webpack, Rspack, Rolldown, Rollup, and Vite. Todo List:
Code Review ResultsIssues & Recommendations
Critical Issues Requiring Immediate Attention1. Rolldown Plugin Incorrect Implementation // Current (WRONG):
zephyr_engine.buildProperties.baseHref = _options.dir;
// Should be:
zephyr_engine.buildProperties.baseHref = _options.publicPath;
// OR implement proper detection like other bundlers 2. Incomplete Rsbuild Plugin
// Suggested implementation:
apply(compiler: any): void {
this._options.zephyr_engine.buildProperties.output = compiler.outputPath;
detectAndStoreBaseHref(this._options.zephyr_engine, compiler);
logBuildSteps(this._options, compiler);
setupZeDeploy(this._options, compiler);
} Performance ConsiderationsMemory Optimization Needed: // Current inefficient approach in ze-basehref-handler.ts:111
Object.values(assetsMap).forEach((asset) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
});
// More efficient:
Object.entries(assetsMap).forEach(([key, asset]) => {
const newAsset: ZeBuildAsset = { ...asset };
// ...
}); Security Analysis✅ No critical security vulnerabilities found
Test Coverage AssessmentStrong: Core basehref handler has excellent test coverage (ze-basehref-handler.spec.ts)
Code Quality Score: 6/10Strengths:
Areas for Improvement:
Top 3 Next Steps:
|
What's added in this PR?
Some customers need a custom path for their file structure, so was requested to add as a zephyr config parameter a custom public path on the snapshots for each one of the bundlers.
Some changes in the zephyr-engine to evaluate basehref parameters and save to snapshot.
Each bundler has a way to pass the basehref as parameter:
The ones with examples are way ahead on testing.
Examples using basehref:
The MF + Rspack example was moved to backlog until further implementation around MF dynamic remote handling with Zephyr.
Screenshots
Webpack example:

render
requests
snapshot
Rspack example

deploy
Vite example

render
snapshot
deploy
What's the issues or discussion related to this PR ?
This work was conducted along @ryok90 to explore the possibilities of each plugin using the default parameters structure pass to zephyr-engine the basehref and the zephyr be fully responsible for the snapshot creation and pointing. So the goal is have less as possible configuration with the plugins, relying the assign behavior to zephyr-engine.
The Basehref + MF + Rspack with Zephyr implementation was moved to backlog until further implementation for dynamic domain handling.
What are the steps to test this PR?
Implement some more basehref with each bundler using vanilla setup or MF with NX.
Documentation update for this PR (if applicable)?
https://webpack.js.org/guides/public-path/
module-federation/core#2633
(Optional) What's left to be done for this PR?
More examples implementation. Other bundlers public path linking
Deeper build testing with the current examples
(Optional) What's the potential risk and how to mitigate it?
Needed to check all the examples execution looking for missing assets.
@zackarychapple @valorkin @ryok90
(Required) Pre-PR/Merge checklist