fix: hack for rspack module federation due to dayjs -> dayjs/esm alias#604
fix: hack for rspack module federation due to dayjs -> dayjs/esm alias#604JounQin wants to merge 2 commits into
dayjs -> dayjs/esm alias#604Conversation
🦋 Changeset detectedLatest commit: bd3f0ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR folds the former theme resource copy step into the main build script and adds a post-build patch to the generated FESM entry to work around an Rspack Module Federation dayjs -> dayjs/esm alias interop issue.
Changes:
- Removed the standalone
scripts/copy-resources.jsscript and related npm scripts. - Added an
afterBuild()post-build step to copy/compile theme SCSS assets. - Added a build-time string patch to rewrite the
dayjsimport in the generatedfesm2022/alauda-ui.mjs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/copy-resources.js | Removed the separate resource copy/SCSS compile script. |
| scripts/build.js | Added post-build resource handling and a dayjs import patch in the build output. |
| package.json | Removed scripts that invoked the deleted standalone build helpers. |
Comments suppressed due to low confidence (1)
scripts/build.js:23
- The Gulp copy/compile streams are started but not awaited. Since
afterBuild()is used to sequence subsequent steps (e.g.,copyDistin watch mode), the build can copy thereleasefolder before theme resources finish writing, resulting in incomplete theme assets in the copied output. Wrap the streams in Promises andawaitthem (e.g., viaPromise.all).
gulp
.src([
'src/theme/_base-var.scss',
'src/theme/_pattern.scss',
'src/theme/_var.scss',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/build.js (2)
53-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSequence
copyDistafterafterBuild()in watch mode.Line 55 kicks off the async patch, but Lines 57-61 immediately copy
release/. In watch mode that means downstream destinations can receive the unpatchedalauda-ui.mjsbefore the dayjs rewrite lands. Please chain the copy step afterafterBuild()settles and surface failures instead of fire-and-forget.Suggested fix
if (watch) { - packagr.watch().subscribe(() => { - afterBuild(); - - if (!isDebug) { - const src = path.resolve(__dirname, '../release'); - const destinations = getBuildDest(); - - destinations.forEach(dest => copyDist(src, dest)); - } - }); + packagr.watch().subscribe({ + next: () => + afterBuild() + .then(() => { + if (!isDebug) { + const src = path.resolve(__dirname, '../release'); + const destinations = getBuildDest(); + + destinations.forEach(dest => copyDist(src, dest)); + } + }) + .catch(err => { + process.exitCode = 1; + throw err; + }), + }); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build.js` around lines 53 - 63, The watch-mode flow currently calls afterBuild() but immediately triggers copying of release/ which can race with the async patch; update the packagr.watch().subscribe handler to wait for afterBuild() to complete (use await or .then on afterBuild() within the subscribe callback) before computing destinations via getBuildDest() and invoking copyDist for each destination, and ensure any errors from afterBuild() or copyDist are surfaced (reject/log) instead of being fire-and-forget so failures in afterBuild() or copyDist are handled.
17-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
afterBuild()wait for the Gulp pipelines to finish.
afterBuild()is now the build-completion hook, but these two streams are started and never awaited. The function can resolve whiletheme/is still being written, so any caller that treatsawait afterBuild()as “artifact is ready” can still race on incomplete assets.Suggested fix
async function afterBuild() { const themeDest = path.resolve(releaseDest, 'theme'); - gulp - .src([ - 'src/theme/_base-var.scss', - 'src/theme/_pattern.scss', - 'src/theme/_var.scss', - 'src/theme/_theme-preset.scss', - 'src/theme/_mixin.scss', - ]) - .pipe(gulp.dest(themeDest)); - - gulp - .src('src/theme/style.scss') - .pipe(sass().on('error', sass.logError)) - .pipe(gulp.dest(themeDest)); + const waitForStream = stream => + new Promise((resolve, reject) => { + stream.on('finish', resolve); + stream.on('error', reject); + }); + + await Promise.all([ + waitForStream( + gulp + .src([ + 'src/theme/_base-var.scss', + 'src/theme/_pattern.scss', + 'src/theme/_var.scss', + 'src/theme/_theme-preset.scss', + 'src/theme/_mixin.scss', + ]) + .pipe(gulp.dest(themeDest)), + ), + waitForStream( + gulp + .src('src/theme/style.scss') + .pipe(sass().on('error', sass.logError)) + .pipe(gulp.dest(themeDest)), + ), + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build.js` around lines 17 - 32, afterBuild currently fires two Gulp streams and returns before they finish; change it to await both pipelines by returning Promise.all of two Promises that resolve on stream completion and reject on error. Specifically, for the pipeline that pipes gulp.src(...).pipe(gulp.dest(themeDest)) and the pipeline that pipes gulp.src('src/theme/style.scss').pipe(sass()...).pipe(gulp.dest(themeDest)), wrap each stream in new Promise((resolve, reject) => stream.on('end', resolve).on('finish', resolve).on('error', reject)) (or use stream.finished) and await Promise.all; keep references to afterBuild, themeDest, gulp.src, sass() and gulp.dest to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build.js`:
- Around line 34-43: The current exact-string replacement for dayjs may silently
not match and leave the file unchanged; update the logic around
esmEntry/esmEntryContent and the write operation to detect whether the
replacement actually occurred and fail the build if not — after performing the
replace on esmEntryContent, compare the result to the original content and if
identical (no replacement) throw an error or call process.exit(1) with a clear
message (e.g., "dayjs import patch did not match in esmEntry") so the build
fails rather than continuing without the federation workaround.
---
Outside diff comments:
In `@scripts/build.js`:
- Around line 53-63: The watch-mode flow currently calls afterBuild() but
immediately triggers copying of release/ which can race with the async patch;
update the packagr.watch().subscribe handler to wait for afterBuild() to
complete (use await or .then on afterBuild() within the subscribe callback)
before computing destinations via getBuildDest() and invoking copyDist for each
destination, and ensure any errors from afterBuild() or copyDist are surfaced
(reject/log) instead of being fire-and-forget so failures in afterBuild() or
copyDist are handled.
- Around line 17-32: afterBuild currently fires two Gulp streams and returns
before they finish; change it to await both pipelines by returning Promise.all
of two Promises that resolve on stream completion and reject on error.
Specifically, for the pipeline that pipes
gulp.src(...).pipe(gulp.dest(themeDest)) and the pipeline that pipes
gulp.src('src/theme/style.scss').pipe(sass()...).pipe(gulp.dest(themeDest)),
wrap each stream in new Promise((resolve, reject) => stream.on('end',
resolve).on('finish', resolve).on('error', reject)) (or use stream.finished) and
await Promise.all; keep references to afterBuild, themeDest, gulp.src, sass()
and gulp.dest to locate the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22401e76-f970-4778-b7dc-99a59d86a5de
📒 Files selected for processing (4)
.changeset/pink-houses-lose.mdpackage.jsonscripts/build.jsscripts/copy-resources.js
💤 Files with no reviewable changes (2)
- scripts/copy-resources.js
- package.json
| // hack for rspack module federation due to `dayjs -> dayjs/esm` alias | ||
| const esmEntry = path.resolve(releaseDest, 'fesm2022/alauda-ui.mjs'); | ||
| const esmEntryContent = await fs.readFile(esmEntry, 'utf-8'); | ||
| await fs.writeFile( | ||
| esmEntry, | ||
| esmEntryContent.replace( | ||
| "import dayjs from 'dayjs';", | ||
| "import dayjs_ from 'dayjs';\nconst dayjs = 'default' in dayjs_ ? dayjs_.default : dayjs_;", | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Fail the build if the dayjs import patch stops matching.
This replacement is exact-string and currently fails open. If ng-packagr changes the emitted import format even slightly, the build will succeed without applying the federation workaround, which is the whole point of this hook.
Suggested fix
// hack for rspack module federation due to `dayjs -> dayjs/esm` alias
const esmEntry = path.resolve(releaseDest, 'fesm2022/alauda-ui.mjs');
const esmEntryContent = await fs.readFile(esmEntry, 'utf-8');
+ const targetImport = "import dayjs from 'dayjs';";
+ const replacement =
+ "import dayjs_ from 'dayjs';\nconst dayjs = 'default' in dayjs_ ? dayjs_.default : dayjs_;";
+ const matches = esmEntryContent.match(/import dayjs from 'dayjs';/g)?.length ?? 0;
+
+ if (matches !== 1) {
+ throw new Error(
+ `Expected exactly one dayjs import to patch in ${esmEntry}, found ${matches}.`,
+ );
+ }
+
await fs.writeFile(
esmEntry,
- esmEntryContent.replace(
- "import dayjs from 'dayjs';",
- "import dayjs_ from 'dayjs';\nconst dayjs = 'default' in dayjs_ ? dayjs_.default : dayjs_;",
- ),
+ esmEntryContent.replace(targetImport, replacement),
);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build.js` around lines 34 - 43, The current exact-string replacement
for dayjs may silently not match and leave the file unchanged; update the logic
around esmEntry/esmEntryContent and the write operation to detect whether the
replacement actually occurred and fail the build if not — after performing the
replace on esmEntryContent, compare the result to the original content and if
identical (no replacement) throw an error or call process.exit(1) with a clear
message (e.g., "dayjs import patch did not match in esmEntry") so the build
fails rather than continuing without the federation workaround.
Summary by CodeRabbit
Bug Fixes
Chores