-
Notifications
You must be signed in to change notification settings - Fork 670
CONSOLE-3853: Optimize module federation of PatternFly packages #13521
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
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. DetailsIn 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 |
spadgett
left a comment
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 { useTranslation } from 'react-i18next'; | ||
| import { Link } from 'react-router-dom'; | ||
| 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 |
jhadvig
left a comment
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 { useTranslation } from 'react-i18next'; | ||
| import { Link } from 'react-router-dom'; | ||
| 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 |
| }, | ||
| "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>/**"]
}
jhadvig
left a comment
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
|
@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. DetailsIn 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. DetailsIn 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. 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/test-infra repository. I understand the commands that are listed here. |
|
@jhadvig: #13521 failed to apply on top of branch "release-4.15": DetailsIn 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: falseandallowFallback: 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-corepackage as an example, it includes adist/dynamicdirectory containingpackage.jsonfiles that refer to specific modules of that package.For example, all
Alertcomponent related code and types (including exports likeAlert,AlertProps,AlertContextetc.) have a corresponding dynamic module at@patternfly/react-core/dist/dynamic/components/Alert/package.json{ "name": "@patternfly/react-core-alert-dynamic", "main": "../../../js/components/Alert/index.js", // => dist/js/components/Alert/index.js "module": "../../../esm/components/Alert/index.js", // => dist/esm/components/Alert/index.js "typings": "../../../esm/components/Alert/index.d.ts", // => dist/esm/components/Alert/index.d.ts "version": "5.1.1", "private": true }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
AlertorWizarddynamic module once or multiple times in different versions.Example use case
@patternfly/react-core5.1.1 - usingAlertandWizardAlertdynamic module [5.1.1] does not exist in shared scope ➡️ add to shared scopeWizarddynamic module [5.1.1] does not exist in shared scope ➡️ add to shared scope@patternfly/react-core5.1.1 - using onlyWizardWizarddynamic module [5.1.1] exists in shared scope ➡️ reuse the one in shared scope@patternfly/react-core5.2.0 - using onlyAlertAlertdynamic 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-iconsThis list can be customized via new
ConsoleRemotePluginoptionsharedDynamicModuleSettings.ConsoleRemotePluginhas 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-webpacknow has a peer dependency"typescript": ">=4.5.5"typescript(dev) dependency that matches the above rangedynamicModuleImportLoaderusing TypeScript compiler API to transform code@patternfly/react-coreand@patternfly/react-icons(dev) dependencies@patternfly/react-coreand@patternfly/react-iconspackagesdynamicModuleImportLoaderwill warn you about any non-index imports detected in your codeSmoke test verification
webpackSharedScopeobject