feat: remove scalprum frontend support - #182
hopehadfield wants to merge 9 commits into
Conversation
Make frontend plugin export use Backstage standard module federation exclusively and remove the legacy Scalprum bundler, commands, options, and dependencies. Clean legacy artifacts from packaged output, preserve NFS feature metadata when available, and update end-to-end expectations for NFS remotes. Assisted-by: gpt-5.6-luna
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 8:39 PM UTC · Completed 9:20 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
🤖 Finished Review · ❌ Failure (validation failed after 1 iteration(s)) · Started 6:45 PM UTC · Completed 6:47 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-5 · Effort: high |
|
🤖 Finished Review · ✅ Success · Started 2:02 PM UTC · Completed 2:22 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $6.17 |
|
Risk Assessment: moderate (2/5) DetailsRe-anchoring to prior score of 2: Tier 1 signals are unchanged (large blast radius, 0 protected paths, 2 dependency files changed), and Tier 2 reveals modest churn driven largely by housekeeping files with 6 fix commits concentrated on paths being deleted. Previous runRisk Assessment: moderate (2/5) DetailsRe-anchoring to prior score of 2: Tier 1 signals are unchanged (large blast radius, 0 protected paths, expected dependency-file changes), and while the new delta introduces a functional modification of the actively-maintained frontend.ts, the overall PR remains a large removal of confirmed-dormant bundler code with no protected paths or security-sensitive changes. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge deletion PR removing stable, dormant bundler code keeps overall risk moderate despite the size and dependency-file changes; the only high-churn file is yarn.lock, which is expected for a dependency removal. |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document the breaking frontend export migration to standard Backstage module federation and NFS metadata. Remove legacy Scalprum options and output references from the supported workflow, and publish the cutover as a major CLI release. Assisted-by: gpt-5.6-luna
Only remove files entries that target dist-scalprum or one of its glob variants, avoiding false positives for unrelated package paths. Assisted-by: gpt-5.6-luna
Keep the recursive-readdir third-party import with the other package imports and separate it from the Node.js path import. Assisted-by: gpt-5.6-luna
|
🤖 Finished Review · ✅ Success · Started 4:38 PM UTC · Completed 4:58 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $4.33 |
| getFullPluginPath(), | ||
| 'dist-dynamic/dist-scalprum/plugin-manifest.json', | ||
| ), | ||
| path.join(getFullPluginPath(), 'dist-dynamic/dist/remoteEntry.js'), |
There was a problem hiding this comment.
Instead of adding tests that verify the dist-scalprum directory no longer exists, we can just remove any expects on the directory entirely. If we no longer carry it as an implementation, I see no need to test its absence.
| ), | ||
| ).toEqual(true); | ||
| // eslint-disable-next-line jest/no-conditional-expect | ||
| expect( |
There was a problem hiding this comment.
Same here, lets just drop it.
| // eslint-disable-next-line jest/no-conditional-expect | ||
| expect(distDynamicPkg.backstage?.features).toEqual(expectedFeatures); | ||
| // eslint-disable-next-line jest/no-conditional-expect | ||
| expect(distDynamicPkg).not.toHaveProperty('scalprum'); |
There was a problem hiding this comment.
Same here, lets just drop it.
There was a problem hiding this comment.
Let's drop any 'scaprum' related assertions here as well.
|
|
||
| // Remove stale Scalprum output that may have been included by npm-packlist | ||
| // before the derived package manifest is customized below. | ||
| await fs.remove(path.join(target, 'dist-scalprum')); |
There was a problem hiding this comment.
Where would this dist-scalprum directory come from? Can we not remove this line and ensure it is never emitted where it is sourced from instead of trying to clean it up here?
| scripts: {}, | ||
| files, | ||
| files: Array.isArray(files) | ||
| ? files.filter((file: string) => !isScalprumFilesEntry(file)) |
There was a problem hiding this comment.
Rather than trying to filter out the Scalprum entries, could we not error directly when such an entry is found? Plugins should no longer be carrying Scalprum-related content any longer, so it would be fine for them to fail so authors are forced to clean up. WDYT?
There was a problem hiding this comment.
Makes sense to me. I removed the cleanup and changed it to fail early when it detects anything scalprum-related.
| } | ||
| : undefined, | ||
| after: pkg => { | ||
| delete (pkg as unknown as Record<string, unknown>).scalprum; |
There was a problem hiding this comment.
Same here; if any Scalprum-related content is found, this should immediately fail an export. We should not try to fix it up for plugin authors.
| `Generating dynamic frontend plugin assets in ${chalk.cyan( | ||
| resolvedScalprumDistPath, | ||
| )}`, | ||
| const legacyScalprumFiles = (await recursive(target)).filter(file => |
There was a problem hiding this comment.
If we fail early, I think we can forgo this check entirely (see comments above).
| return normalized === 'true' || normalized === '1' || normalized === 'yes'; | ||
| } | ||
|
|
||
| function isScalprumFilesEntry(file: string): boolean { |
There was a problem hiding this comment.
Perhaps a simple file.includes('dist-scalprum') would suffice? We don't really care about glob semantics, and we're only targeting a minimal set of packages.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Keep the e2e coverage focused on the standard module federation output and feature metadata rather than asserting the absence of removed implementation details. Assisted-by: gpt-5.6-luna
Use the minimal dist-scalprum substring check for the small set of supported frontend packages, as suggested in review.
Fail frontend exports when plugin metadata, files entries, or output directories still reference Scalprum instead of silently filtering or cleaning them.
|
|
🤖 Review · ❌ Terminated · Started 9:29 PM UTC · Ended 9:51 PM UTC Commit: |
|
Updated with your comments @jonkoops, thanks for the review! Looks like we have some removals to do on the plugins side before this gets in... |
| true, | ||
| ) | ||
| .option('--no-generate-module-federation-assets', '', false) | ||
| .action(lazy(() => import('./export-dynamic-plugin').then(m => m.command))); |
There was a problem hiding this comment.
[high] breaking-cli
Five options are removed from rhdh-cli plugin export: --scalprum-config, --generate-scalprum-assets, --no-generate-scalprum-assets, --generate-module-federation-assets, and --no-generate-module-federation-assets. Any external CI pipeline, Makefile, or shell script that passes any of these flags will receive an 'unknown option' error and the export will abort.
Suggested fix: Remove all five options from rhdh-cli plugin export invocations. Standard module-federation assets are now generated unconditionally — no flag is needed.
| const { name, files } = originalPkg; | ||
|
|
||
| if (!opts.generateScalprumAssets && !opts.generateModuleFederationAssets) { | ||
| if ('scalprum' in originalPkg) { |
There was a problem hiding this comment.
[high] breaking-contract
The scalprum field in a plugin's package.json was previously consumed as inline Scalprum configuration during plugin export. It now causes an immediate hard error. Any plugin package that includes scalprum: {...} will encounter a hard export failure when the CLI is upgraded to 3.0.0.
Suggested fix: Remove the scalprum key from each plugin's package.json before upgrading to rhdh-cli 3.0.0. Scalprum module exposure configuration is no longer applicable; module federation is handled automatically.
| if (opts.clean) { | ||
| await fs.remove(path.join(paths.targetDir, 'dist')); | ||
| } | ||
| const legacyScalprumFile = Array.isArray(files) |
There was a problem hiding this comment.
[medium] breaking-contract
If a plugin's npm files array contains any glob that includes dist-scalprum, or if a dist-scalprum/ directory exists on disk in the plugin's target directory, plugin export now throws a hard error before any build step executes. Plugins that have not removed Scalprum artifacts from their package manifest and working tree will be blocked from exporting.
Suggested fix: Remove any dist-scalprum globs from plugin files fields and delete checked-in or stale dist-scalprum/ directories before running rhdh-cli plugin export with 3.0.0.
| if (fs.existsSync(path.join(targetPath, 'dist'))) { | ||
| configSchemaPaths.push(path.join(targetPath, 'dist/.config-schema.json')); | ||
| } | ||
| configSchemaPaths = [path.join(targetPath, 'dist/.config-schema.json')]; |
There was a problem hiding this comment.
[low] error-handling-idiom
The guard that previously protected the config-schema write path was removed. The new code unconditionally assigns configSchemaPaths. If dist-dynamic/dist does not exist, the subsequent fs.writeJson throws ENOENT because fs-extra's writeJson does not create parent directories.
Suggested fix: Use fs.outputJson instead of fs.writeJson, or add await fs.ensureDir(path.dirname(configSchemaPath)) before the write loop.
|
🤖 Finished Review · ✅ Success · Started 9:29 PM UTC · Completed 9:51 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-4-6 · Effort: high · Cost: $6.72 |
https://redhat.atlassian.net/browse/RHIDP-15353
Make frontend plugin export use Backstage standard module federation exclusively and remove the legacy Scalprum bundler, commands, options, and dependencies. Clean legacy artifacts from packaged output, preserve NFS feature metadata when available, and update end-to-end expectations for NFS remotes.
Assisted-by: gpt-5.6-luna