Skip to content

Conversation

@vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented Jun 12, 2019

This PR aims to improve the overall quality of Console plugins by detecting common problems at build time, using Jest as the test platform.

fail-ModelFeatureFlag

In the above screenshot, metal3-plugin was modified to add VirtualMachineModel which is already contributed by kubevirt-plugin. The object diff reflects the expectation of duplicateModels being [].

Tests under packages/console-app/src/__tests__/extension-checks work with the same set of active plugins as the webpack build. This ensures consistency between plugins getting tested vs. plugins getting bundled in the Console build output.

Summary of changes

  • Jest testRegex now matches files with spec.(ts|tsx|js|jsx) suffix, allowing developers to put test utility modules under __tests__ directories
  • added @types/jest dev dependency along with corresponding tsconfig.json update
  • ⭐ code under packages now uses lodash instead of lodash-es
  • ⭐ using webpack.NormalModuleReplacementPlugin to replace lodash with lodash-es
  • plugin SDK codegen improved to support dynamic loading of plugins along with proper test coverage
  • extension properties docs improved (NavItem, YAMLTemplate)

Consequently, production code that interprets Console extensions is simplified, i.e. no need to report on conflicting models or similar problems.

The demo plugin was updated so that extension tests pass when this plugin is enabled.

⭐ What's the deal with lodash vs. lodash-es anyway?

lodash-es is a variation of the lodash library which uses ECMAScript module syntax.

Most tools, including ts-node and Jest, expect code under the node_modules directory to be transpiled and ready for use. Using libraries like lodash-es in environments like Node.js LTS results in syntax errors. Console integration-tests, for example, use lodash exactly because of this (Protractor not configured to transpile node_modules/lodash-es, as opposed to Jest).

To be able to dynamically load Console plugin entry points in context of Node.js env. (Jest), we have to use lodash instead of lodash-es. To retain current approach of using lodash-es in the webpack build, we use NormalModuleReplacementPlugin. This allows us to remove the lodash vs. lodash-es duality.


/cc @spadgett @alecmerdler @christianvogt @jelkosz

@openshift-ci-robot
Copy link
Contributor

@vojtechszocs: GitHub didn't allow me to request PR reviews from the following users: jelkosz.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

This PR aims to improve the overall quality of Console plugins by detecting common problems at build time, using Jest as the test platform.

Tests under packages/console-app/src/__tests__/extension-checks work with the same set of active plugins as the webpack build. This ensures consistency between plugins getting tested vs. plugins getting bundled in the Console build output.

Summary of changes

  • Jest testRegex now matches files with spec.(ts|tsx|js|jsx) suffix, allowing developers to put test utility modules under __tests__ directories
  • added @types/jest dev dependency along with corresponding tsconfig.json update
  • ⭐ code under packages now uses lodash instead of lodash-es
  • ⭐ using webpack.NormalModuleReplacementPlugin to replace lodash with lodash-es
  • plugin SDK codegen improved to support dynamic loading of plugins along with proper test coverage
  • extension properties docs improved (NavItem, YAMLTemplate)

Consequently, production code that interprets Console extensions is simplified, i.e. no need to report on conflicting models or similar problems.

The demo plugin was updated so that extension tests pass when this plugin is enabled.

⭐ What's the deal with lodash vs. lodash-es anyway?

lodash-es is a variation of the lodash library which uses ECMAScript module syntax.

Most tools, including ts-node and Jest, expect code under the node_modules directory to be transpiled and ready for use. Using libraries like lodash-es in environments like Node.js LTS results in syntax errors. Console integration-tests, for example, use lodash exactly because of this.

To be able to dynamically load Console plugin entry points in context of Node.js env. (Jest), we have to use lodash instead of lodash-es. To retain current approach of using lodash-es in the webpack build, we use NormalModuleReplacementPlugin. This allows us to remove the lodash vs. lodash-es duality.


/cc @spadgett @alecmerdler @christianvogt @jelkosz

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.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 12, 2019
@vojtechszocs
Copy link
Contributor Author

As a follow-up, I'd like to replace all lodash-es imports with lodash, but I'm curious what @spadgett thinks.

Alternatively, we can retain the existing lodash vs. lodash-es duality and require all Console plugins to have their entry points (i.e. src/plugin.ts) not reference lodash-es (directly or indirectly).

@spadgett
Copy link
Member

As a follow-up, I'd like to replace all lodash-es imports with lodash, but I'm curious what @spadgett thinks.

Just to be clear, we'd be able to use NormalModuleReplacementPlugin and retain tree shaking?

@spadgett
Copy link
Member

Great to see these additional tests 👍

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2019
@spadgett spadgett added this to the v4.2 milestone Jun 13, 2019
@vojtechszocs
Copy link
Contributor Author

As a follow-up, I'd like to replace all lodash-es imports with lodash, but I'm curious what @spadgett thinks.

Just to be clear, we'd be able to use NormalModuleReplacementPlugin and retain tree shaking?

Yes 😃 AFAIK we still need to use lodash-es with webpack to do proper tree shaking, since lodash-es uses the native ES module syntax which webpack can easily look into & optimize.

The NormalModuleReplacementPlugin is used to replace modules which share the same exports. With this PR, during webpack build, import * as _ from 'lodash'; becomes import * as _ from 'lodash-es';. If we had code like import { uniq } from 'lodash'; it would become import { uniq } from 'lodash-es';.

Also, if you can think of any additional extension tests that we should perform, let me know and I'll add them. The more issues with plugins we can check during test/build time (instead of runtime), the better.

@vojtechszocs vojtechszocs mentioned this pull request Jun 13, 2019
4 tasks
@vojtechszocs
Copy link
Contributor Author

@spadgett To ensure this doesn't have unexpected impact on build outputs, I'll run yarn analyze (webpack-bundle-analyzer) before and after the PR and compare the bundle reports.

@vojtechszocs
Copy link
Contributor Author

@spadgett To ensure this doesn't have unexpected impact on build outputs, I'll run yarn analyze (webpack-bundle-analyzer) before and after the PR and compare the bundle reports.

Hmm, it seems like public/dist/stats.json gets malformed when generated via yarn analyze command:

Using ^[[1m1 worker^[[22m with ^[[1m2048MB^[[22m memory limit
{
// usual JSON output goes here
}

So I've compared yarn build outputs between

Generated chunk sizes were equivalent. The main entry point has the same combined asset size of 2.14 MiB. This means the build outputs are still the same as before.

@vojtechszocs
Copy link
Contributor Author

Please note, this PR isn't complete without its follow-up #1724 which removes lodash-es imports from the codebase.

@spadgett
Copy link
Member

Please note, this PR isn't complete without its follow-up #1724 which removes lodash-es imports from the codebase.

Can it merge without #1724? I see you updated eslintrc for lodash

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@spadgett
Copy link
Member

@christianvogt It might be better for you to review if you have bandwidth

@vojtechszocs
Copy link
Contributor Author

Please note, this PR isn't complete without its follow-up #1724 which removes lodash-es imports from the codebase.

Can it merge without #1724? I see you updated eslintrc for lodash

Yes, this PR alone should pass tests and the webpack build output should be the same as before, effectively using lodash-es for the compilation.

#1724 completes this PR in terms of Lodash imports used within the code & ESLint configuration, but this has no impact on webpack output.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@vojtechszocs
Copy link
Contributor Author

Rebased & included config tweaks from #1724

frontend/packages/.eslintrc.js

 'no-restricted-imports': [
   'error',
   {
     name: 'lodash-es',
-    message: 'Use lodash instead.',
+    message: 'Use lodash instead. webpack is configured to use lodash-es automatically.',
   },
 ],

frontend/webpack.config.ts

-    new webpack.NormalModuleReplacementPlugin(/^lodash$/, 'lodash-es'),
+    // replace 'lodash' and 'lodash/*' imports with a lodash-es equivalent
+    new webpack.NormalModuleReplacementPlugin(/^lodash($|\/.*$)/, (resource) => {
+      resource.request = resource.request.replace(/^lodash/, 'lodash-es');
+    }),

@vojtechszocs
Copy link
Contributor Author

Need to rebase on top of #1743, one more update incoming.

@vojtechszocs
Copy link
Contributor Author

Need to rebase on top of #1743, one more update incoming.

Done, please review the second commit titled Add plugin integration tests.

@vojtechszocs
Copy link
Contributor Author

#1724 rebased & updated, please review if you have some spare time 😃

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, spadgett, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit a8de8fa into openshift:master Jun 19, 2019
@mareklibra
Copy link
Contributor

mareklibra commented Jun 19, 2019

@vojtechszocs , With #1708 merged, recent master fails to open vms list from kubevirt-plugin.
The issue is within import of kubevirt-web-ui-components, investigating ...

Edit: import './style.scss'; seized to work as well.

@vojtechszocs
Copy link
Contributor Author

@vojtechszocs , With #1708 merged, recent master fails to open vms list from kubevirt-plugin.
The issue is within import of kubevirt-web-ui-components, investigating ...

Edit: import './style.scss'; seized to work as well.

I'm looking into this.

@vojtechszocs
Copy link
Contributor Author

@mareklibra #1766 should fix the problem you've encountered.

@logonoff logonoff deleted the plugin-tests branch September 25, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants