Skip to content
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

Add support for static plugins #1499

Merged

Conversation

vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented Apr 26, 2019

Static plugins

This PR adds initial support for static plugins to extend the core functionality of Console web application.

Note: any file paths mentioned below are meant in context of the frontend directory.

Monorepo updates

New packages:

  • @console/internal - existing code in public directory
  • @console/plugin-sdk - plugin SDK to be used by plugins as well as the main application
  • @console/demo-plugin - demo plugin to showcase plugin SDK capabilities (disabled by default)

Following diagram shows the current monorepo package dependency graph:

monorepo-pkg-diagram

The @console/internal package has no dependencies because it's not meant to be part of the monorepo structure. Its code is expected to be moved to either @console/app or @console/shared package.

The @console/plugin-sdk package is meant to support both writing and interpreting Console plugins. Any common code needed by this package should be moved to @console/shared.

The @console/demo-plugin package is used to demonstrate plugin SDK capabilities in a synthetic way. It's not meant to be a real-world example, but rather a starting reference for Console plugin developers as well as potential candidate for e2e tests (ensure expected behavior from user perspective). Any common code needed by this plugin (or any future plugins) should be moved to @console/shared.

The @console/active-plugins package is generated during the webpack build based on the current plugin metadata. This package provides a list of plugins to be included in the application.

The demo plugin is disabled by default.

Writing a plugin

Add new package to represent your plugin, e.g. packages/my-plugin.

  • your package.json file should look similar to packages/console-demo-plugin/package.json
  • make sure to use @console scope for your package name, e.g. @console/my-plugin

Plugin metadata is stored in the package.json file under the consolePlugin key.

{
  "name": "@console/my-plugin",
  "version": "0.0.0-fixed",
  "description": "Example plugin",
  "private": true,
  // scripts, dependencies, etc.
  "consolePlugin": {
    "entry": "src/plugin.ts"
  }
}

The consolePlugin.entry path should point to a module that exports the plugin object. If not defined or not a string, your package won't be recognized as a Console plugin.

Next, add the packages/my-plugin/src/plugin.ts module that exports your plugin.

import { Plugin, HrefNavItem, ResourceNSNavItem } from '@console/plugin-sdk';

type ConsumedExtensions = HrefNavItem | ResourceNSNavItem;

const plugin: Plugin<ConsumedExtensions> = [
  {
    type: 'NavItem/Href',
    properties: {
      section: 'Home',
      componentProps: {
        name: 'Test Href Link',
        href: '/test',
      },
    },
  },
  {
    type: 'NavItem/ResourceNS',
    properties: {
      section: 'Workloads',
      componentProps: {
        name: 'Test ResourceNS Link',
        resource: 'pods',
      },
    },
  },
];

export default plugin;

For better type checking and code completion, always use a Plugin type parameter that represents the union of all the extension types consumed by your plugin.

// This is BAD
const plugin: Plugin<Extension<any>> = [ /* extensions */ ];

// This is GOOD
const plugin: Plugin<HrefNavItem | ResourceNSNavItem> = [ /* extensions */ ];

// This is also GOOD
type ConsumedExtensions = HrefNavItem | ResourceNSNavItem;
const plugin: Plugin<ConsumedExtensions> = [ /* extensions */ ];

From code perspective, a plugin is simply a list of extensions.

Each extension is a realization (instance) of an extension type using the parameters provided via the properties object. Core extension types should follow Category or Category/Specialization format, e.g. NavItem/Href.

Extension's properties may be used to contain both data and code. To reference plugin-specific modules or React components, use the import function to facilitate proper code splitting.

// This is BAD
{
  type: 'ResourcePage/List',
  properties: {
    model: ExampleModel,
    loader: () => Promise.resolve(ExamplePage)
  }
}

// This is GOOD
{
  type: 'ResourcePage/List',
  properties: {
    model: ExampleModel,
    loader: () => import('./components/example' /* webpackChunkName: "my-plugin" */).then(m => m.ExamplePage)
  }
}

The value of webpackChunkName should reflect the name of your plugin.

Plugin activation

To activate your plugin, simply open packages/console-app/package.json and add it to dependencies.

 "dependencies": {
   "@console/internal": "0.0.0-fixed",
+  "@console/my-plugin": "0.0.0-fixed",
   "@console/plugin-sdk": "0.0.0-fixed",
   "@console/shared": "0.0.0-fixed"
 }

Rebuild the application (e.g. yarn dev) and reload it in the browser. You should now see the following log message:

1 plugins active

Make sure that packages/console-app/package.json file contains all the plugins that should be active in production builds and commit this change to git.

Active plugins are detected automatically during the webpack build.

Using the VirtualModulesPlugin, new @console/active-plugins module is dynamically generated.

var activePlugins = [];
import plugin_0 from '@console/my-plugin/src/plugin.ts';
activePlugins.push(plugin_0);
// process any other active plugins
export default activePlugins;

Application code then imports this module and uses it to create the plugin registry.

// public/plugins.ts
const activePlugins = (process.env.NODE_ENV !== 'test')
  ? require('@console/active-plugins').default as PluginList
  : [];

Plugin registry

A plugin registry object is used to aggregate and query for Console extensions, utilizing TypeScript's type guards feature to narrow a variable to a more specific type.

// items is NavItem[]
// NavItem is HrefNavItem | ResourceNSNavItem
const items = registry.getNavItems('Workloads');

for (const item of items) {
  if (isHrefNavItem(item)) {
    // isHrefNavItem narrows item's type to HrefNavItem
  }
}

When interpreting extensions from within a React component, it's always good to avoid using component state in favor of simpler techniques such as memoization.

Follow-ups

  • address TODO(vojtech) comments 🍰
  • instead of referencing the plugin registry, pass a "plugin store" to the relevant React components
    • plugin store provides access to the registry and additional operations, such as adding new extensions at runtime (CRD-based UI extensions can use the same mechanism)
    • can be realized as a higher-order component similar to e.g. connectToFlags
    • a change in registry data should trigger an update of the corresponding component subtree

/cc @spadgett @jhadvig @christianvogt @priley86 @mareklibra @rawagner @jelkosz

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 26, 2019
@vojtechszocs vojtechszocs force-pushed the static-plugins branch 2 times, most recently from e4f10bc to 419396f Compare April 26, 2019 18:33
@vojtechszocs
Copy link
Contributor Author

vojtechszocs commented Apr 26, 2019

Q & A

Q: Extension.type is a string. Why not use enums?

A: To support the "plugin extends plugin" use case where a plugin may contribute new extension types, the only way to extend an existing enum is through the declaration merging feature.

// foo.ts
export enum Color {
  Red = 'Red',
  Blue = 'Blue',
}
// bar.ts
import { Color } from './foo';

// augment the existing module declaration
declare module './foo' {
  export enum Color {
    Green = 'Green',
  }
}
// qux.ts
import { Color } from './foo';
import from './bar'; // apply the augmentation

const c = Color.Green;

However, at runtime, console.log(c) shows an undefined value, which is weird so I've decided not to use enums at this point.


Q: Why must I restart webpack (yarn dev) when adding or removing plugins or updating console-app's package.json file?

A: I wrote an experimental webpack plugin to watch custom files (packages/*/package.json) and call the VirtualModulesPlugin.writeModule function to make the @console/active-plugins module reflect the current state.

However, the content of the generated module seems to be cached across webpack compilations so I've decided not to do @console/active-plugins re-generation at this point.

@@ -36,7 +37,7 @@
".(ts|tsx|js|jsx)": "./node_modules/ts-jest/preprocessor.js"
},
"transformIgnorePatterns": [
"<rootDir>/node_modules/(?!lodash-es/.*)"
"<rootDir>/node_modules/(?!lodash-es|@console)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, this change tells Jest to transform (basically "un-ignore") any @console module paths, in addition to existing lodash-es module paths.

More specifically, without this change, following module dependency chain:

__tests__/components/resource-pages.spec.tsx
public/components/resource-pages.ts
public/plugins.ts

will throw syntax errors (export * from statements) since public/plugins.ts imports from @console/plugin-sdk and Jest doesn't transform stuff in node_modules by default.

This is also the reason why all Console monorepo packages should use the @console scope in their name.

@vojtechszocs
Copy link
Contributor Author

vojtechszocs commented Apr 26, 2019

/cc @bparees

@vojtechszocs
Copy link
Contributor Author

Updated the Console monorepo package diagram.

@vojtechszocs vojtechszocs mentioned this pull request Apr 29, 2019
3 tasks
@spadgett
Copy link
Member

Hey @vojtechszocs, great to see this progress.

Do we need @console/internal? I'd suggest just moving everything from public into @console/app, then pull things from there into @console/shared as needed. Moving things temporarily into an internal package guarantees everything needs to be moved at least twice and likely adds unnecessary churn.

@vojtechszocs
Copy link
Contributor Author

Do we need @console/internal?

The @console/internal package was created by simply adding public/package.json and updating the monorepo workspace definition:

   "workspaces": [
-    "packages/*"
+    "packages/*",
+    "public"
   ],

This PR does not git-move existing files under the public directory.

The reason of adding the @console/internal package is to allow better imports across different packages, until the public directory gets removed. For example, in Console application entry point:

// packages/console-app/src/index.ts

// relative imports can get ugly
import '../../../public/components/app';

// package imports are cleaner
import '@console/internal/components/app';

I'd suggest just moving everything from public into @console/app, then pull things from there into @console/shared as needed.

I'll do a separate PR that git-moves stuff from public into the @console/app package.

Moving things temporarily into an internal package guarantees everything needs to be moved at least twice and likely adds unnecessary churn.

This PR does not git-move existing files under the public directory.

export function getActivePluginsModule(packageFiles: string[]): string {
const { appPackage, pluginPackages } = readPackages(packageFiles);
let output = `
var activePlugins = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

var => const
Even though I think the generated code is only used in a virtual file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the @console/active-plugins module source was generated as part of the webpack afterCompile hook, at which point the code was expected to be transformed, hence the use of var instead of const.

Currently, the @console/active-plugins module source is generated as part of webpack config object creation, i.e. ahead of webpack compiler execution, which means that we can use the modern ES (or TS) syntax.

Thanks for pointing this out. I'll change it to const activePlugins.

Even though I think the generated code is only used in a virtual file?

Correct. The getActivePluginsModule function returns the source of @console/active-plugins module, which is a purely virtual module.

@@ -227,6 +229,10 @@ const NavSection = connect(navSectionStateToProps)(
this.setState({isOpen: expandState});
}

getPluginNavItems = memoize(
(section) => plugins.registry.getNavItems(section)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to avoid starting the pattern of using stateful globals in a component. Much prefer passing in a prop as it makes components easier to test when they rely only on props.
withExtensionRegistry or withPlugins HOC

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed reading your Follow-ups section.
I'm ok with doing this in stages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to avoid starting the pattern of using stateful globals in a component.

I agree completely 👍

This is just a temporary step between "no link to plugins" and "React-proper link to plugins".

In a follow-up PR, we will formalize how React components access the plugin registry.

@christianvogt
Copy link
Contributor

LGTM

@@ -117,7 +119,13 @@ export const resourceDetailPages = ImmutableMap<GroupVersionKind | string, () =>
.set(referenceForModel(InstallPlanModel), () => import('./operator-lifecycle-manager/install-plan' /* webpackChunkName: "install-plan" */).then(m => m.InstallPlanDetailsPage))
.set(referenceForModel(ClusterOperatorModel), () => import('./cluster-settings/cluster-operator' /* webpackChunkName: "cluster-operator" */).then(m => m.ClusterOperatorDetailsPage))
.set(referenceForModel(ClusterVersionModel), () => import('./cluster-settings/cluster-version' /* webpackChunkName: "cluster-version" */).then(m => m.ClusterVersionDetailsPage))
.set(referenceForModel(OAuthModel), () => import('./cluster-settings/oauth' /* webpackChunkName: "oauth" */).then(m => m.OAuthDetailsPage));
.set(referenceForModel(OAuthModel), () => import('./cluster-settings/oauth' /* webpackChunkName: "oauth" */).then(m => m.OAuthDetailsPage))
.set(referenceForModel(OAuthModel), () => import('./cluster-settings/oauth' /* webpackChunkName: "oauth" */).then(m => m.OAuthDetailsPage))
Copy link
Contributor

@mareklibra mareklibra May 3, 2019

Choose a reason for hiding this comment

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

Not sure I understand this. @vojtechszocs , can you please briefly explain these two .set() lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing it out. I'll remove the duplicity.

"private": true,
"main": "src/index.ts",
"scripts": {
"test": "yarn --cwd ../.. run test packages/console-plugin-sdk"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about e2e (integration tests), where do they fit?
Will we still collocate them on a single place considering they test whole application? Or split them per plugin?

The first option sounds better to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about e2e (integration tests), where do they fit?
Will we still collocate them on a single place considering they test whole application?

Yes, I think that's the general consensus. Unit tests, on the other hand, should be co-located with their corresponding package.


export interface NavItemProperties {
// TODO(vojtech): link to existing nav sections by value
section: 'Home' | 'Workloads';
Copy link

Choose a reason for hiding this comment

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

These need to get updated to include all current sections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, there's a TODO item for that.

I'd like to define some section title type (union of strings, like above) and use it in both NavSection and here.

frontend/package.json Show resolved Hide resolved
}
}

function isValidPluginPackage(pkg: Package): pkg is PluginPackage {
Copy link
Member

Choose a reason for hiding this comment

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

nit: we typically use function expressions in console:

Suggested change
function isValidPluginPackage(pkg: Package): pkg is PluginPackage {
const isValidPluginPackage = (pkg: Package): pkg is PluginPackage => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll keep that in mind for my future changes.

Personally, I find the typical function declaration more readable:

function name(args): returnType { body }

vs.

const name = (args): returnType => { body }

anyway, I'll try to stick with existing conventions used in Console code.

@vojtechszocs
Copy link
Contributor Author

Follow-up PR that adds model-based feature flag extension capability: #1528

@vojtechszocs
Copy link
Contributor Author

/cc @alecmerdler

/**
* Registry used to query for Console extensions.
*/
export class ExtensionRegistry {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking forward to seeing this removed in favor of a HOC.

*
* @todo(vojtech) write ESLint rule to guard against extension type duplicity
*/
export interface Extension<P> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use type here to discourage using classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In TypeScript, type aliases provide a name to refer to any kind of type, while interfaces are used to name object specific types.

Interface types have many similarities to type aliases for object type literals, but since interface types offer more capabilities they are generally preferred to type aliases.

An interface can be named in an extends or implements clause, but a type alias for an object type literal cannot.

An interface can have multiple merged declarations, but a type alias for an object type literal cannot.

When describing object types, I'd stick to the TS spec recommendation and prefer using interfaces over type aliases. Using interfaces does not imply using classes to implement those interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

An interface can be named in an extends or implements clause, but a type alias for an object type literal cannot.

Not true. You can still implement a type; unless the type definition uses a union operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type TestObj = {
  x: number;
  y: () => boolean;
};

// valid
class Foo implements TestObj {
  x = 1;
  y() { return true; }
}

@christianvogt You are correct - you can use object literal type alias (e.g. TestObj) in the implements clause. It's also possible to do object composition via the intersection (&) operator:

type AnotherTestObj = TestObj & {
  z: string;
};

However, the spec is still correct in that you can't use e.g. TestObj in the extends clause, more specifically when declaring a class:

// valid
type Bar = <T extends TestObj>(obj: T) => void;

// invalid
class Qux extends TestObj {}

Still, I'd prefer using interfaces, as they describe the shape of an object (they're not a traditional OOP interface whose usage is coupled with classes).

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @vojtechszocs. This is a big step!

There don't seem to be any major concerns and several LGTMs. I'm going to tag it for merge. We can continue iterating and making it better.

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, rawagner, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2019
@openshift-merge-robot openshift-merge-robot merged commit 982fb74 into openshift:master-next May 4, 2019
@spadgett spadgett added this to the v4.2 milestone May 16, 2019
@vojtechszocs vojtechszocs deleted the static-plugins branch May 20, 2019 16:56
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants