Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,7 @@ const parseDeps = (
missingDepCallback: MissingDependencyCallback,
) => {
const srcDeps = { ...pkg.devDependencies, ...pkg.dependencies };

depNames
// Console does not have an explicit react-router-dom(-v5-compat) dependency.
// react-router-dom(-v5-compat) shared module impl. delegates to react-router.
.filter((name) => name !== 'react-router-dom-v5-compat')
.filter((name) => name !== 'react-router-dom')
.filter((name) => !srcDeps[name])
.forEach(missingDepCallback);

depNames.filter((name) => !srcDeps[name]).forEach(missingDepCallback);
return _.pick(srcDeps, depNames);
};

Expand All @@ -97,12 +89,20 @@ const parseDepsAs = (
const parseSharedModuleDeps = (pkg: PackageJson, missingDepCallback: MissingDependencyCallback) =>
parseDeps(
pkg,
sharedPluginModules.filter(
(m) =>
m !== '@openshift/dynamic-plugin-sdk' && // This is a direct SDK dependency as well as a shared module
sharedPluginModules.filter((m) => {
const { allowFallback, aliased } = getSharedModuleMetadata(m);

return (
// Exclude Console plugin SDK shared modules.
!m.startsWith('@openshift-console/') &&
!getSharedModuleMetadata(m).allowFallback,
),
// This is a Console plugin SDK dependency and a shared module.
m !== '@openshift/dynamic-plugin-sdk' &&
// Exclude modules for which a plugin provided fallback version is disallowed.
// Also exclude modules whose implementation is aliased to another module.
!allowFallback &&
!aliased
Comment on lines +100 to +103
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
// 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

);
}),
missingDepCallback,
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import type { getSharedScope } from '../plugin-shared-modules';
import { monkeyPatchSharedScope } from '../plugin-shared-modules';

describe('monkeyPatchSharedScope', () => {
it('adds aliased modules to share scope object', () => {
const getModule = jest.fn();

const testScope: ReturnType<typeof getSharedScope> = {
'react-router': {
'7.13.1': { from: 'openshift-console', eager: true, loaded: 1, get: getModule },
},
};

monkeyPatchSharedScope(testScope);

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']);
Comment on lines +16 to +27
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

});
});
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const registerLegacyPluginEntryCallback = () => {

// eslint-disable-next-line no-console
console.warn(
`[DEPRECATION WARNING] ${pluginName} was built for an older version of Console and may not work correctly in this version.`,
`[WARNING] ${pluginName} was built for an older version of Console and may not work correctly in this version.`,
);

window[REMOTE_ENTRY_CALLBACK](patchedPluginName, entryModule);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,34 @@ const SHARED_SCOPE_NAME = 'default';
*/
export const initSharedScope = async () => __webpack_init_sharing__(SHARED_SCOPE_NAME);

/**
* webpack provided type of `__webpack_share_scopes__` object is incorrect; at runtime,
* each shared module comes with one or more version(s) with each version scoped to its
* own object, for example:
*
* ```js
* {
* [SCOPE_NAME]: {
* 'react': {
* '18.3.1': {
* from: 'openshift-console',
* eager: true,
* loaded: 1,
* get: moduleFactoryFunction,
* }
* }
* }
* }
* ```
*/
type WebpackShareScopes = {
[scopeName: string]: {
[moduleName: string]: {
[moduleVersion: string]: typeof __webpack_share_scopes__[string][string];
};
};
};

/**
* Get the webpack share scope object.
*/
Expand All @@ -20,7 +48,8 @@ export const getSharedScope = () => {
throw new Error('Attempt to access share scope object before its initialization');
}

return __webpack_share_scopes__[SHARED_SCOPE_NAME];
// TODO: Remove this type cast once __webpack_share_scopes__ object type is fixed
return (__webpack_share_scopes__[SHARED_SCOPE_NAME] as unknown) as WebpackShareScopes[string];
};

/**
Expand All @@ -29,8 +58,7 @@ export const getSharedScope = () => {
* - add `react-router-dom-v5-compat` module aliased to `react-router`
* - add `react-router-dom` module aliased to `react-router`
*/
export const monkeyPatchSharedScope = () => {
const scope = getSharedScope();
export const monkeyPatchSharedScope = (scope = getSharedScope()) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does scope need to be typed now?

Suggested change
export const monkeyPatchSharedScope = (scope = getSharedScope()) => {
export const monkeyPatchSharedScope = (scope: WebpackShareScopes = getSharedScope()) => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

scope['react-router-dom'] = scope['react-router'];
scope['react-router-dom-v5-compat'] = scope['react-router'];
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ type SharedModuleMetadata = Partial<{
*/
allowFallback: boolean;

/**
* If `true`, this module's implementation is aliased to another module.
*
* Plugins should avoid using aliased modules due to risk of potential skew between
* aliased vs actual module code.
*
* @default false
*/
aliased: boolean;

/**
* A message describing the deprecation, if the module has been deprecated.
*
Expand Down Expand Up @@ -57,8 +67,8 @@ const sharedPluginModulesMetadata: Record<SharedModuleNames, SharedModuleMetadat
'react-i18next': {},
'react-redux': {},
'react-router': {},
'react-router-dom': { deprecated: 'Use react-router instead.' },
'react-router-dom-v5-compat': { deprecated: 'Use react-router instead.' },
'react-router-dom': { aliased: true, deprecated: 'Use react-router instead.' },
'react-router-dom-v5-compat': { aliased: true, deprecated: 'Use react-router instead.' },
redux: {},
'redux-thunk': {},
};
Expand All @@ -72,7 +82,8 @@ export const getSharedModuleMetadata = (
const {
singleton = true,
allowFallback = false,
aliased = false,
deprecated = false,
} = sharedPluginModulesMetadata[moduleName];
return { singleton, allowFallback, deprecated };
return { singleton, allowFallback, aliased, deprecated };
};
Original file line number Diff line number Diff line change
Expand Up @@ -135,15 +135,17 @@ export const validateConsoleExtensionsFileSchema = (
return new SchemaValidator(description).validate(schema, extensions);
};

const getDeprecatedSharedModuleWarnings = (pkg: ConsolePluginPackageJSON): string[] => {
const getCompileTimeSharedModuleWarnings = (pkg: ConsolePluginPackageJSON): string[] => {
const warnings: string[] = [];

sharedPluginModules.forEach((moduleName) => {
const { deprecated } = getSharedModuleMetadata(moduleName);
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`,
);
Comment on lines +142 to 149
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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*.

Comment on lines +146 to 149
Copy link
Copy Markdown
Member

@logonoff logonoff Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
});
Expand Down Expand Up @@ -476,7 +478,7 @@ export class ConsoleRemotePlugin implements WebpackPluginInstance {
}
}

getDeprecatedSharedModuleWarnings(this.pkg).forEach((message) => {
getCompileTimeSharedModuleWarnings(this.pkg).forEach((message) => {
compilation.warnings.push(new compiler.webpack.WebpackError(message));
});
});
Expand Down