Improve handling of aliased Console provided shared modules#16376
Improve handling of aliased Console provided shared modules#16376vojtechszocs wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
plugin-api-changed
vojtechszocs
force-pushed
the
improve-aliased-shared-module-handling
branch
from
c91a3f9 to
3aaa97e
Compare
📝 WalkthroughWalkthroughThis changeset refines shared module handling in the console dynamic plugin SDK. It introduces stricter dependency validation in the package definition parser, adds type safety for webpack share scopes, and implements a runtime aliasing mechanism for 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Review rate limit: 8/10 reviews remaining, refill in 10 minutes and 1 second. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-shared-modules.spec.ts`:
- Around line 16-27: The test currently uses structural equality checks for
aliases; change the two alias assertions that compare
testScope['react-router-dom'] and testScope['react-router-dom-v5-compat'] to
testScope['react-router'] from using toEqual to using toBe so the test asserts
identity (same object) rather than deep equality; update the assertions in
plugin-shared-modules.spec.ts where testScope is compared so both alias checks
use toBe.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts`:
- Around line 142-149: The warning selection logic in the ConsoleRemotePlugin
uses a ternary that checks deprecated first, which causes aliased warnings
(aliased from getSharedModuleMetadata for moduleName) to be shadowed; update the
logic in the block that calls getSharedModuleMetadata and hasPackageDependency
so aliased is checked before deprecated (or push both messages explicitly) —
e.g., change the conditional used in warnings.push that currently references
deprecated ? ... : ... to first check aliased and emit the alias-skew message
for aliased modules, otherwise emit the deprecated message, ensuring the code
paths in getSharedModuleMetadata, aliased, deprecated, hasPackageDependency, and
warnings.push correctly surface alias warnings for react-router-dom*.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a8462503-dcf0-4d34-a5cd-144d309402ff
📒 Files selected for processing (6)
frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-shared-modules.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.tsfrontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.tsfrontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts
📜 Review details
🔇 Additional comments (5)
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-init.ts (1)
83-86: Warning prefix update looks fine.Using the generic
[WARNING]prefix keeps the legacy callback aligned with the broader shared-module warning wording.frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-shared-modules.spec.ts (1)
1-8: Verify the type-only import compiles here.
ReturnType<typeof getSharedScope>usually needs a value import, so please confirm this passes under the repo's TypeScript settings or switch to a value import / exported scope type.frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
79-80: Dependency filtering now matches the new metadata model.Surfacing missing deps while excluding aliased shared modules from
peerDependencieslooks consistent with the runtime aliasing change.Also applies to: 92-107
frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts (1)
20-28: Metadata additions look consistent.Adding
aliasedhere gives the downstream warning and peerDependency filters a single source of truth.Also applies to: 70-88
frontend/packages/console-dynamic-plugin-sdk/src/runtime/plugin-shared-modules.ts (1)
15-64: The helper refactor looks good.The explicit share-scope typing and optional
scopeparameter keep the runtime behavior intact while making the alias patching easier to test.
| expect(Object.keys(testScope)).toEqual([ | ||
| 'react-router', | ||
| 'react-router-dom', | ||
| 'react-router-dom-v5-compat', | ||
| ]); | ||
|
|
||
| expect(testScope['react-router']).toEqual({ | ||
| '7.13.1': { from: 'openshift-console', eager: true, loaded: 1, get: getModule }, | ||
| }); | ||
|
|
||
| expect(testScope['react-router-dom']).toEqual(testScope['react-router']); | ||
| expect(testScope['react-router-dom-v5-compat']).toEqual(testScope['react-router']); |
There was a problem hiding this comment.
Assert alias identity, not just structural equality.
toEqual would still pass if the aliases were copied instead of pointing at the same module map. Use toBe for the alias checks so this test actually covers the wiring.
🛠️ Suggested test tweak
- expect(testScope['react-router-dom']).toEqual(testScope['react-router']);
- expect(testScope['react-router-dom-v5-compat']).toEqual(testScope['react-router']);
+ expect(testScope['react-router-dom']).toBe(testScope['react-router']);
+ expect(testScope['react-router-dom-v5-compat']).toBe(testScope['react-router']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/runtime/__tests__/plugin-shared-modules.spec.ts`
around lines 16 - 27, The test currently uses structural equality checks for
aliases; change the two alias assertions that compare
testScope['react-router-dom'] and testScope['react-router-dom-v5-compat'] to
testScope['react-router'] from using toEqual to using toBe so the test asserts
identity (same object) rather than deep equality; update the assertions in
plugin-shared-modules.spec.ts where testScope is compared so both alias checks
use toBe.
| const { deprecated, aliased } = getSharedModuleMetadata(moduleName); | ||
|
|
||
| if (deprecated && hasPackageDependency(pkg, moduleName)) { | ||
| if ((deprecated || aliased) && hasPackageDependency(pkg, moduleName)) { | ||
| warnings.push( | ||
| `[DEPRECATION WARNING] Console provided shared module ${moduleName} has been deprecated: ${deprecated}`, | ||
| deprecated | ||
| ? `[WARNING] Console provided shared module ${moduleName} has been deprecated: ${deprecated}` | ||
| : `[WARNING] Console provided shared module ${moduleName} is aliased, beware of potential skew between aliased vs actual module code`, | ||
| ); |
There was a problem hiding this comment.
Alias warnings are currently shadowed.
These modules still have a truthy deprecated message in metadata, so this ternary will keep emitting the old deprecation text and never the new alias-skew warning for react-router-dom*.
If the build should surface the alias message, branch on aliased first or emit both warnings explicitly.
🔧 Possible fix
- if ((deprecated || aliased) && hasPackageDependency(pkg, moduleName)) {
- warnings.push(
- deprecated
- ? `[WARNING] Console provided shared module ${moduleName} has been deprecated: ${deprecated}`
- : `[WARNING] Console provided shared module ${moduleName} is aliased, beware of potential skew between aliased vs actual module code`,
- );
+ if (aliased && hasPackageDependency(pkg, moduleName)) {
+ warnings.push(
+ `[WARNING] Console provided shared module ${moduleName} is aliased, beware of potential skew between aliased vs actual module code`,
+ );
+ } else if (deprecated && hasPackageDependency(pkg, moduleName)) {
+ warnings.push(
+ `[WARNING] Console provided shared module ${moduleName} has been deprecated: ${deprecated}`,
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts`
around lines 142 - 149, The warning selection logic in the ConsoleRemotePlugin
uses a ternary that checks deprecated first, which causes aliased warnings
(aliased from getSharedModuleMetadata for moduleName) to be shadowed; update the
logic in the block that calls getSharedModuleMetadata and hasPackageDependency
so aliased is checked before deprecated (or push both messages explicitly) —
e.g., change the conditional used in warnings.push that currently references
deprecated ? ... : ... to first check aliased and emit the alias-skew message
for aliased modules, otherwise emit the deprecated message, ensuring the code
paths in getSharedModuleMetadata, aliased, deprecated, hasPackageDependency, and
warnings.push correctly surface alias warnings for react-router-dom*.
| deprecated | ||
| ? `[WARNING] Console provided shared module ${moduleName} has been deprecated: ${deprecated}` | ||
| : `[WARNING] Console provided shared module ${moduleName} is aliased, beware of potential skew between aliased vs actual module code`, | ||
| ); |
There was a problem hiding this comment.
Currently the aliased shared module warning is never shown (as both react-router-dom and react-router-dom-v5-compat are both deprecated).. can we make the assumption that all aliased shared modules are also deprecated and remove this check?
There was a problem hiding this comment.
A shared module that is aliased doesn't necessarily have to be deprecated - I think these are orthogonal concepts.
You are correct that react-router-dom and react-router-dom-v5-compat modules are currently both aliased and deprecated.
This code is meant to print just one warning per each aliased and/or deprecated module, with deprecation having higher priority.
I'll think about this some more but the general idea was to keep track of which modules are aliased (in addition to tracking their deprecated status).
| */ | ||
| export const monkeyPatchSharedScope = () => { | ||
| const scope = getSharedScope(); | ||
| export const monkeyPatchSharedScope = (scope = getSharedScope()) => { |
There was a problem hiding this comment.
Does scope need to be typed now?
| export const monkeyPatchSharedScope = (scope = getSharedScope()) => { | |
| export const monkeyPatchSharedScope = (scope: WebpackShareScopes = getSharedScope()) => { |
There was a problem hiding this comment.
TypeScript will infer the type of scope based on default parameter initializer getSharedScope().
The inferred type of getSharedScope() expression is WebpackShareScopes[string] instead of the suggested WebpackShareScopes (the latter represents the object containing one or more share scope objects - in Console we use just one default share scope object).
{
[moduleName: string]: {
[moduleVersion: string]: { /* module data for name@version */ };
};
}We could explicitly type scope as WebpackShareScopes[string] but I don't think it's necessary due to above mentioned type inference.
| // Exclude modules for which a plugin provided fallback version is disallowed. | ||
| // Also exclude modules whose implementation is aliased to another module. | ||
| !allowFallback && | ||
| !aliased |
There was a problem hiding this comment.
nit
| // Exclude modules for which a plugin provided fallback version is disallowed. | |
| // Also exclude modules whose implementation is aliased to another module. | |
| !allowFallback && | |
| !aliased | |
| // Exclude modules for which a plugin provided fallback version is disallowed. | |
| !allowFallback && | |
| // Also exclude modules whose implementation is aliased to another module. | |
| !aliased |
|
/label px-approved |
openshift-ci
Bot
added
px-approved
|
@vojtechszocs: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Analysis / Root cause
Some of Console provided shared modules are aliased for backwards compatibility with older Console plugins.
react-router-domreact-routerreact-router-dom-v5-compatreact-routerWe should improve existing code which handles aliased shared modules.
Solution description
monkeyPatchSharedScopeto ensure all aliases are set up as expectedgetSharedScopehas the right type - webpack provided__webpack_share_scopes__type is incorrectScreenshots / screen recording
N/A - no visual impact
Test cases
window.webpackSharedScopebefore vs after PR - should be the same objectreact-router-domand/orreact-router-dom-v5-compatto dependencies - should emit warning(s)Reviewers and assignees
/assign @logonoff
Summary by CodeRabbit
New Features
Bug Fixes