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
CONSOLE-3853: Optimize module federation of PatternFly packages #13521
CONSOLE-3853: Optimize module federation of PatternFly packages #13521
Conversation
@vojtechszocs: This pull request references CONSOLE-3853 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
); | ||
|
||
if (!modifiedModules.includes(moduleRequest) && transformImports(moduleRequest)) { | ||
normalModule.loaders.push({ |
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.
This is where our custom loader gets registered for modules that match the transformImports
filter.
webpack loaders are applied in reverse order, so the last one runs first.
This code was inspired by https://github.com/artembatura/modify-source-webpack-plugin
/retest |
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.
Nice work on this 👍 No major concerns from me. I'll give @jhadvig and others a chance to review.
/approve
import { MonitoringIcon } from '@patternfly/react-icons/dist/esm/icons/monitoring-icon'; | ||
import { MonitoringIcon } from '@patternfly/react-icons'; |
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.
We'll make sure we document the recommended way to do these imports.
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.
@spadgett @jhadvig The dynamic plugin README is already quite big.
I think it makes sense to add a separate Markdown doc to document Console plugin shared modules, including PatternFly dynamic modules. What do you think?
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.
So I think it would be best to keep it tin the dynamic plugin README, even if its will make it bigger.
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.
OK, will update the README.
const dynamicModulePathToPkgDir = glob | ||
.sync(`${basePath}/dist/dynamic/**/package.json`) | ||
.reduce<Record<string, string>>((acc, pkgFile) => { | ||
// eslint-disable-next-line |
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.
nitpick: better to only disable the eslint rule we need
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.
@spadgett The require
call (line below) actually triggers multiple ESLint errors:
72:19 error Calls to require() should use string literals import/no-dynamic-require
72:19 error Unexpected require() global-require
72:19 error A `require()` style import is forbidden @typescript-eslint/no-require-imports
72:19 error Require statement not part of import statement @typescript-eslint/no-var-requires
so I didn't specify the rule names explicitly. Such require
calls always tend to trigger a lot of ESLint errors.
Is it OK to leave it like so for simplicity?
We do this also in some other parts of the codebase, e.g. packages/console-dynamic-plugin-sdk/src/runtime/plugin-manifest.ts
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.
+1 leaving as is
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.
Thank you for adding the tests 👍
/lgtm QE Approver: |
/label px-approved |
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.
🤯 🤯 🤯
Awesome work @vojtechszocs Looking forward for the demo 🙌
/lgtm
import { MonitoringIcon } from '@patternfly/react-icons/dist/esm/icons/monitoring-icon'; | ||
import { MonitoringIcon } from '@patternfly/react-icons'; |
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.
So I think it would be best to keep it tin the dynamic plugin README, even if its will make it bigger.
/retest |
/label docs-approved |
adding labels manually since they are not picked up by the bot |
/retest |
@@ -11,5 +11,12 @@ | |||
"skipLibCheck": true, | |||
"lib": ["dom", "es7", "es2017.string"] | |||
}, | |||
"include": ["src"] | |||
"include": ["src"], | |||
"ts-node": { |
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.
This makes debugging the Console webpack code (triggered via demo plugin webpack build) much easier, paired with the following VS Code launch config:
{
"name": "Example",
"type": "node",
"request": "launch",
"runtimeExecutable": "node",
"runtimeArgs": ["--nolazy", "-r", "ts-node/register/transpile-only"],
"args": ["node_modules/.bin/webpack"],
"cwd": "${workspaceRoot}",
"internalConsoleOptions": "openOnSessionStart",
"skipFiles": ["<node_internals>/**"]
}
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, rhamilto, spadgett, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The dynamic demo plugin could be loaded without any issue, all plugin fictional is work as normal, no new CSS issue found |
@vojtechszocs: This pull request references CONSOLE-3853 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
This PR addresses the following item of CONSOLE-3883 (Improve Console dynamic plugin documentation)
|
/label acknowledge-critical-fixes-only |
/hold Revision 6c9b2ea was retested 3 times: holding |
/retest |
/cherry-pick release-4.15 |
@jhadvig: once the present PR merges, I will cherry-pick it on top of release-4.15 in a new PR and assign it to you. In response to this:
Instructions 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/test-infra repository. |
/hold cancel |
/retest |
/retest |
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. Instructions 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/test-infra repository. I understand the commands that are listed here. |
@jhadvig: #13521 failed to apply on top of branch "release-4.15":
In response to this:
Instructions 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/test-infra repository. |
[ART PR BUILD NOTIFIER] This PR has been included in build openshift-enterprise-console-container-v4.16.0-202402021641.p0.g74c31e3.assembly.stream for distgit openshift-enterprise-console. |
Since openshift/console#13521 merged, plugins are expected to have an explicit @patternfly/react-icons dependency.
Motivation
Console web application provides specific shared modules to its dynamic plugins.
Most of these shared modules are configured as follows:
singleton: true
)allowFallback: false
)While this configuration favors code consistency, it's not flexible enough for PatternFly packages.
Starting with 4.15 release, all PatternFly v4 shared modules are configured as
singleton: false
andallowFallback: true
(#12983). This allows existing plugins using these PatternFly v4 shared modules to work as expected. At some point in future, these PatternFly v4 shared modules will be deprecated and eventually removed.In order to share PatternFly v5+ modules in an optimal way, this PR implements the concept of shared dynamic modules.
Dynamic Modules
Some vendor packages may support dynamic modules to be used with webpack module federation.
Taking
@patternfly/react-core
package as an example, it includes adist/dynamic
directory containingpackage.json
files that refer to specific modules of that package.For example, all
Alert
component related code and types (including exports likeAlert
,AlertProps
,AlertContext
etc.) have a corresponding dynamic module at@patternfly/react-core/dist/dynamic/components/Alert/package.json
In the plugin code, this dynamic module can then be imported like so:
Let's say the plugin also uses some other PatternFly components:
Once we configure webpack module federation to treat each of these dynamic modules as shared modules like so:
this will effectively allow us to load the same
Alert
orWizard
dynamic module once or multiple times in different versions.Example use case
@patternfly/react-core
5.1.1 - usingAlert
andWizard
Alert
dynamic module [5.1.1] does not exist in shared scope ➡️ add to shared scopeWizard
dynamic module [5.1.1] does not exist in shared scope ➡️ add to shared scope@patternfly/react-core
5.1.1 - using onlyWizard
Wizard
dynamic module [5.1.1] exists in shared scope ➡️ reuse the one in shared scope@patternfly/react-core
5.2.0 - using onlyAlert
Alert
dynamic module [5.2.0] does not exist in shared scope ➡️ add to shared scopeDynamic module processing
This PR includes a parser (
getDynamicModuleMap
) that detects all dynamic modules of the given vendor package.The default list of vendor packages for which dynamic modules should be processed is:
@patternfly/react-core
@patternfly/react-icons
This list can be customized via new
ConsoleRemotePlugin
optionsharedDynamicModuleSettings
.ConsoleRemotePlugin
has been modified to perform the following tasks:Impact on existing plugins
This PR attempts to be as backwards compatible as possible and provide reasonable defaults for all dynamic plugins.
Here is the list of checks and minor changes that plugins should address:
@openshift-console/dynamic-plugin-sdk-webpack
now has a peer dependency"typescript": ">=4.5.5"
typescript
(dev) dependency that matches the above rangedynamicModuleImportLoader
using TypeScript compiler API to transform code@patternfly/react-core
and@patternfly/react-icons
(dev) dependencies@patternfly/react-core
and@patternfly/react-icons
packagesdynamicModuleImportLoader
will warn you about any non-index imports detected in your codeSmoke test verification
webpackSharedScope
object