[WIP] Add extensible components API to create components which can expose API and render endpoints

[karelhala]

TODO

• Create new infrastructure to register new extensible component
• Add tests to cover this functionality

Usage

If we want to expose some functionality to other plugin/JS code we can use this API to register new component with callbacks to render functions and API functions. These callbacks should hold this to surrounding class so we can manipulate it's data from outside.

To create new component simply call ManageIQ.extensionComponents.newComponent with name, object with api callbacks and object with render callbacks.

Example

var toolbarComponent = ManageIQ.extensionComponents.newComponent('toolbar', {
      triggerClick: function(item) {
        this.items.filter(function(localItem){return localItem.name === item.name}).click();
      }.bind(this),
      triggerSomething: function(item) { this.onSomething(item) }.bind(this)
    }, {
      newButtonPlaceholder: function(button) {
        return this.$element.find('.custom-buttons');
      }.bind(this)
    });

Usage in plugin

var subscriber = ManageIQ.extensionComponents.subscribe('toolbar');
subscriber.with(toolbarActions);

function toolbarActions(component) {
 //Do something with API callbacks and renderCallbacks
}

When the component is destroyed you should also unsubscribe itself (by calling delete on returned object of subscribe)so there are no memory leaks happening. So given you used var subscriber = ManageIQ.extensionComponents.subscribe('toolbar'); this is how to unsubscribe.

onComponentDestroy() {
  subscriber.delete();
}

Additional info

To observe which components are registered you can look at object ManageIQ.extensionComponents.items which is iterable set of components.

When component is deregistered it should also remove itself from extension items, this can be done by toolbarComponent.unsubscribe()

[karelhala]

@miq-bot add_label enhancement

[karelhala]

@martinpovolny this PR will add new common pack which will have every part needed for registering new extensible component. Example on how to use it from plugin can be seen at martinpovolny/miq_plugin_example#5 it's also required to add react, react-dom, loader for jsx and declare React as window property, described also in said PR.

[karelhala]

@vojtechszocs btw this is based on your propositions.
@himdel, @ohadlevy ping

[vojtechszocs]

I'd prefer () => void to narrow down the function signature.

[vojtechszocs]

I'd suggest using a simpler name, e.g. ManageIQ.extensions.

This extension API can be used by both visual and non-visual components, which can originate from both core and plugins.

[vojtechszocs]

I'd also suggest to have the MiQ extension API expose a minimal usable set of objects/functions.

Just like with proposed Redux changes here:

ManageIQ.redux = {
  store,
  addReducer,
  applyReducerHash
};

Notice that in above snippet, we don't expose impl. details like rootReducer etc.

[vojtechszocs]

This is the API through which clients should be registering their extensions.

In this context, a "component" can be both visual or non-visual element of the application. For example, a toolbar component vs. a router (UI navigation) component.

I'd suggest to name this method addComponent to reflect the fact that we're adding a new extension.

[vojtechszocs]

Can we please upgrade to the latest RxJS version and use next? 😃

[vojtechszocs]

Also, it would be nice to have a TS type representing items we add to source.

[karelhala]

New version of RxJS is hard to include in this PR, I don't like importing it because it will make this pack 2MB big, and we have on window older version of it 😢 but new PR to correspond this change would be nice (for entire miq-ui-classic).

[vojtechszocs]

I think the source is pretty similar to our Redux rootReducer so I don't think we should expose it to clients.

Instead, we should expose useful API methods like addComponent and subscribe that internally work with source.

[karelhala]

Well the is source works as event emiter, you'll pass newComponent to it and it will notify everyone that new component was added, items are just persistent object so when someone starts to listen on source later on, it can check which components has been registered and extend them.

[karelhala]

But the source can be local variable so no need to expose it to outside world.

[vojtechszocs]

I'd suggest to have this as:

const items: Set = new Set();
ManageIQ.extensions.getItems() = () => items;

[vojtechszocs]

API object is an object that defines interaction with the given component.

For example, an extensible toolbar component would expose API object providing getItems method. Or providing addItem method that adds new item to the toolbar component.

Interaction through API object is DOM agnostic. For interaction on the DOM level, one should use the render object (below).

[vojtechszocs]

Looking at MiQ plugin example using this PR, I assume the render object serves the purpose of visual (DOM level) integration, correct?

However, I'd really suggest using the concept of render functions instead of DOM element getters.

A render function is a function which takes a DOM element and renders something into it:

function renderStuff(element) { ... }

The render parameter here should be an object containing render functions, using following interface signature:

[propName: string]: (element: HTMLElement) => void;

Example code using a render function:

// component is obtained through subscribe API
component.render.addButton((element) => {
  // actual render logic goes here, e.g. ReactDOM.render
});

Any interaction that doesn't involve DOM should be done through API object (above).

[karelhala]

I'd put both getPlaceToRender and standalone render function at same level (render callbacks). I guess naming this render is wrong, how about domCallbacks? Generally we want to enable both render from component itself and get element to render inside plugin.

[vojtechszocs]

Well, the render object was meant as a hook to the DOM tree owned by the given component. This object would contain render callbacks - functions of type (element: HTMLElement) => void.

tableComponent.render.actionButton((element) => ...)
tableComponent.render.customFooter((element) => ...)

The element argument represents the DOM container to be used for extending the component's own DOM tree. The element is owned by the component itself, acting as a bridge between element's own DOM and the custom extension's DOM.

For example, imagine an Angular component with following DOM:

<div class='miq-table'> <!-- component root element -->
  <div class='miq-table-buttons'>
    <button>one</button> <button>two</button>
    <span /> <!-- bridge for render.actionButton -->
  </div>
  <div class='miq-table-content />
  <div class='miq-table-footer>
    <span>foo text</span>
    <span /> <!-- bridge for render.customFooter -->
  </div>
</div>

The functions are designed as callbacks, which gives the component control over when they are called, because the component itself knows best when to call them, and if to call them at all.

If we expose the component's DOM element (e.g. getPlaceToRender), we risk the chance that component's own DOM tree gets out of sync with given DOM element (imagine a case where a table component would hide its footer). I think it's better to just use the render callbacks with meaningful names & giving control over their execution to the component itself. @karelhala what do you think?

[karelhala]

For simple html changes, it's good. However if some plugin would like to use their own render function (for example react) we'd loose this option.

I'd allow both of these functions:

1. simple HTML render like creating new input which can be either some HTML partial or object with name, ID, etc..
2. for complex rendering I'd allow return of placeholder element. In this callback function we can have some logic which will check if it was called (so some other plugin won't break anything).

Developer of such plugin has to know that using such functions can break something and it's his responsibility to change something. However with second function we have a lot of advantages so we should consider if we really don't want to use it. For me it feels less invasive to pass placeholder element and let plugin render some stuff inside it, rather than doing something like this:

let placeholder = document.createElement('div');
ReactDOM.render(<someCmp />, placeholder);

var subs = subscribe('toolbar');
subs.with((component) => component.renderSomeContent(placeholder));

Where renderSomeContent is function for rendering HTML partial inside extended component. I don't believe it will behave correctly and that data changes will be refreshed correctly.

However I understand that the second option for complex examples should not be part of render object, rather to be part of API object.

So the question here is. Do we want to render just plain HTML partials or let plugins render it themselves via their own logic? I'd go for both options.

[vojtechszocs]

Do we really need the return value, and if so, why?

[karelhala]

When creating and adding new component new component will also has unbind function to deregister itself.

[vojtechszocs]

If source is internal, as I suggested above, we'd have full control over what is pushed into it.

[vojtechszocs]

I'd name this component instead of event.

[vojtechszocs]

The above code is used to synchronize source with items.

Wouldn't it be better to have source as Set and have an RxJS observable on top of that?

[karelhala]

This code is for removing components which are no longer on screen, so we can see which component is active on given screen at given time.

[vojtechszocs]

I'd just log the error instead of throwing.

[vojtechszocs]

This code is to catch any future additions of given extension, right?

[karelhala]

This is for plugins and other pieces of JS to say that they want to have control over certain component with given name. If any component with let's say toolbar is registered at time 0 and some component wants to change it at time 10 the component is taken from items however if component with name toolbar is registered at time 15 callback to component which wants to use such extComponent is used in subscribe. So this is basically function for both listening on source and for checking items which are already present on screen.

[vojtechszocs]

This code is to invoke existing extensions, right?

[vojtechszocs]

There's only add action for now, but what about remove?

The general idea is that e.g. Angular component would make itself extensible within its $onInit hook, and do the opposite within its $onDestroy hook.

[karelhala]

Unsubscribe function is part of created component, so by calling addComponent you will receive component object which has unsubscribe function in it. So no need to use remove action via rxjs.

[vojtechszocs]

@karelhala please see my review comments. I can push some commits but first, let me know what you think.

[mtho11]

LGTM

[mtho11]

Any reason not to use ES6 => instead of function

[karelhala]

@vojtechszocs thank you for review. I fixed some of your proposals 👍

[ohadlevy]

I wonder if we should use webpack import (e.g.)

import {subscribe, addComponent} from 'extensible-component';

IMHO we should expose it as a global var, but going forward I think its better to leverage webpack as you would get compile errors and live reloading when needed.

[vojtechszocs]

The idea was that MiQ APIs are primarily exposed through globals, e.g. window.ManageIQ.stuff.

Given that, we also defined client libraries like lib.ts to be compiled into lib.js and published to npm registry, so that client JS code can do import { addComponent } from 'miq-lib'.

(I'd prefer those client libraries to be named client-extension and client-redux, instead of lib, but it's not a big deal.)

[ohadlevy]

@vojtechszocs again, I'm not against exposing those API's but legacy / others, but I wonder if its better to use import / export with webpack vs relay on window (for example for testing, live reload functionality etc).

[vojtechszocs]

@ohadlevy normally you'd not use the globals, you'd just use import { addComponent } from 'miq-lib'. This applies to both core- and plugin- JS code. All functions in lib should delegate to the corresponding global object.

[karelhala]

The idea behind this is that both redux and extensible component will be npm packages, pulled either from rails plugin or just included in ui-classic. So importing will look like import { addComponent } from '@manageIq/extension'. However it's convenient to have this in ui-classic the way it is now, so we don't have to rely on npm package and such, so for now it's easier to develop this way and move it to npm package later on.

About the global variables, that's mostly just for testing/legacy reasons. Plus when bundling webpack packages right now we'd duplicate a lot of code. So let's say we have 2 plugins both are using Rx.js, webpack will then create packs which are nearly 2 MB big, both packs will have same rx included in them.

[ohadlevy]

thanks @himdel I think we are both agreeing on all of your points.

I think what I'm saying, is that within webpack managed code, we should default to import vs using the globals.

[karelhala]

@ohadlevy the biggest problem here is that both redux and extensible component code is not bundled as a library. They sit in ui-classic for easier development and are treated as general packs. But once we have proper setup of these two libraries (plus commons chunk plugin set up in ui-components) and they'll be npm packages we (and any plugin using webpack) can import them as you said.

[vojtechszocs]

we should default to import vs using the globals

Agreed on that. We use the ManageIQ global variable as a place to store and reference the newly added mechanisms for any JS code out there. In ES6/webpack compatible code, we always prefer import over direct global access.

[ohadlevy]

@karelhala why is it different if its a npm package or not? as long as its being exported (via an npm package or on ui-classic/webpack) it should work just the same.

ideally, going forward, new code will land in webpack control, so it shouldnt be really used that much (e.g. using globals).

when you actually move it to a library, all you would need to change is your import line, e.g.

import { something } from './some-pack'

to

import { something } from 'some-package'

[karelhala]

I think we are in circles and talking about same things.

Yes if we are in webpack managed file we will favor webpack imports over global access variables. That's why we have lib files here - you will include addNewComponent which will reference to global variable, and once we move all files to webpack managed world we can just change the lib files in a way that they will use imported functions as wel.

[vojtechszocs]

@karelhala thank you for addressing my review comments! I'll go through the changes.

[vojtechszocs]

Question, why do we need the component && part here?

Edit: OK, I see that we do this to ensure component is both defined and has the correct name.

[miq-bot]

Checked commits karelhala/manageiq-ui-classic@6d7d3e2~...1aaa1e3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

[vojtechszocs]

Do we want to mix multiple test frameworks together?

That's generally a bad idea.

Isn't jasmine good enough?

It's good enough for simple projects where you handle things like ES6 transpilation manually. However, this has a tendency to introduce test env. into your build tools, complicating your existing build (e.g. webpack) configuration.

Good testing tools handle things like ES6 transpilation for you.

Btw from what I looked jest looks really great, so I would vote for multiple test frameworks, but I think setting this up should be part of different PR.

Jest includes Jasmine as its test runner and adds many useful testing facilities, including snapshot testing, mocks and handy expectations (assertions).

[ohadlevy]

@karelhala @vojtechszocs whats the status of this PR?

TBH: I wonder if it doesn't make more sense just to get the redux parts in initially (so people can actually start using them within the same code base) and in parallel finalize usage implementations (such as forms) and external "API" ?

[karelhala]

@ohadlevy well I think we have to decide whether or not we want this to be part of ui-classic codebase or we want to create custom repo with some utils helpers. I think we should definitely go for utils repo, but it's a long way there (and a lot of work - CC, travis settings, tests and other tools).

So may I suggest that we merge redux to classic-ui first (new pull request with much cleaner code and couple of tests on top). This PR however can be discussed further more and can be merged to ui-classic as well (when we feel it's ok). Once we agree on utils repo (what should be part of it and such) we could move both redux and extension API to it.

[himdel]

@karelhala I'm rather confused about this.

However much I look, this doesn't do anything, does it? I mean, this just provides an interface that components can inherit from, and if they do, you can call a function which adds something to an array in that component's instance .. and that's it. Am I right?

Seems like an overkill to introduce 200 lines of code to ..do nothing, since each extensible component still needs to handle those 2 arrays manually - feels like "let's create an abstraction and then figure out how to use it and what for" :).

What am I missing? :)

---

Also, the example in the description doesn't tell me anything:

• how is that toolbar component defined in the first place? - I'm guessing it needs to do something special to trigger all those triggerClick, triggerSomething and newButtonPlaceholder
• what does that newComponent function actually return? (I'm guessing it's not a new component object, since you'd have to manually change the selector then to be able to actually use it... .. unless this is React-only?)
• is this for react components or angular components?
• does it change all the instances of that component everywhere, or does it change just one instance?
• this really needs a proper example, including rationale..

Also, I still fail to see any use case of this which can realistically work and not be implemented more simply by inheriting the component and overriding methods, or extending the component to take such a config as an proper argument.

[karelhala]

@himdel well, there are 2 things - one thing to communicate data trough data API, the other one is to add option to render inside component using any render function you wish for. It's just a way to show how things should be done, to simplify such extension and provide a way how to render inside component in a way that component itself knows when and where to render.

[himdel]

Aaah, thanks @karelhala, I understand now :)...

This is a mechanism our extensible components (or anything else) can use to "publish" their API within the manageiq app, not a mechanism that actually touches components in any way.

=> blocker: Please let's not call this component. We're already using that word to mean something completely different, let's use a different name here :). "Extension point" maybe? "Extension API"?

(In fact, during all the Mahwah presentations, I though they were about actual components, so I'm probably not alone.)

(But note that a real example would still come in handy.. for example I only think that in toolbarActions you can call component.triggerClick() if you need to do that - but maybe it's this.triggerClick() instead? Or under a sub-object? Definitely needs to be mentioned explicitly.)

[vojtechszocs]

This is a mechanism our extensible components (or anything else) can use to "publish" their API within the manageiq app, not a mechanism that actually touches components in any way.

@himdel exactly. The "anything else" part is important - basically any kind of existing, logical object (in UI context) that we'd like to make extensible. Making an object extensible = allowing other code to interact with it (and possibly enhance it in some way).

This PR proposes two kinds of interaction:

• direct function calls (applies to any kind of extensible objects) - extensible object exposes "API" hash, containing functions that take data and/or callbacks, for example

toolbarApi.addItem(title, onClick)

In this example, you have an imaginary Toolbar component exposing toolbarApi with addItem function. Calling toolbarApi.addItem has the (immediate) effect of adding a new item to the Toolbar. This kind of interaction doesn't involve DOM extension, it's effectively a direct communication with the extensible object.

• DOM extension (applies only to visual components) - extensible object exposes "render" hash, containing functions that allow you to register DOM hooks, for example

function domHook (container) {
  ReactOrWhateverYouLike.renderStuffInto(container);
}
toolbarRender.customLabel(domHook)

What happens here is we tell Toolbar component that whenever it renders its DOM, it should also run domHook on the appropriate "container" DOM element. (The "container" element is owned by Toolbar and is meant to be the DOM root of the particular extension, in this case a custom label.)

React-based impl. of imaginary Toolbar would look like this:

// set through toolbarRender.customLabel
let customLabelHook = undefined

const Toolbar = ({ items }) => (
  <div className='toolbar-main'>
    {/* render the usual component stuff */}
    {items.map(item => (
      <ToolbarItem key={item.id} text={item.text} />
    ))}
    {/* invoke DOM hooks, if defined */}
    {customLabelHook && customLabelHook(
      <div /> 
    )}
  </div>
)

The difference between "API" and "render" is that with "render" you are registering a DOM hook, whereas with "API" you are making an (immediate) function call on the extensible object.

=> blocker: Please let's not call this component. We're already using that word to mean something completely different, let's use a different name here :). "Extension point" maybe? "Extension API"?

I agree with @himdel.

MIQ UI contains different kinds of objects, some of them are (Angular) components. Each of those objects can be made extensible.

By making an object extensible, we'll create an "extension point" for that object. The "extension point" has two parts, both of which are optional (but at least one of them should be defined): "API" and "render", as described above. The "render" part applies only to visual objects (components), the "API" part can be applied to any kind of objects.

(Imagine you have a SPA-like page with Router object or invisible component, it can still be made extensible by exposing "API", but without the "render" part.)

Also, @karelhala I noticed that

These callbacks should hold this to surrounding class so we can manipulate it's data from outside.

I wouldn't recommend to rely on this, instead let's just have a very clear way of interaction/extension through either "API" or "render" (or both). Manipulating data from outside is basically the purpose of "API" part.

[vojtechszocs]

So may I suggest that we merge redux to classic-ui first (new pull request with much cleaner code and couple of tests on top).

👍 let's have a cleaned-up PR (with some tests) that adds basic Redux infra. It should also include an example of how to plug an existing component (like a form) into Redux. Let's start with something really simple and proceed from there.

[vojtechszocs]

let's have a cleaned-up PR (with some tests) that adds basic Redux infra

@karelhala @himdel I'd like to work on this

[himdel]

Thanks both of you, makes sense :).

But given all that.. adding back WIP, since it looks like this is not the final PR.

[himdel]

@miq-bot add_label wip

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[vojtechszocs]

Hello, I've got a minimal Redux API working (based on Karel's work with some improvements), I'll add some Jasmine unit tests and will submit a new PR.

I'm using ng-redux (for actual store impl.) and plain redux (basic impl. suitable for testing) based on Karel's recent code.

[ohadlevy]

@vojtechszocs whats the state of this? is there a new PR ? thanks.

[ohadlevy]

@karelhala @vojtechszocs I've seen a PR at theforeman/foreman#4925 that leverages https://github.com/randallknutson/redux-injector - might be useful here as well.

[vojtechszocs]

@vojtechszocs whats the state of this? is there a new PR ? thanks.

I had some troubles with Jasmine tests (JS generated from app/javascript/packs wasn't served properly) so I'll check my developer setup and debug it via bundle exec rake environment jasmine in the browser, thanks to @karelhala and @himdel for pointing me to the right direction.

A minimal Redux infra code (with all TypeScript & JSDoc goodness) is already done, I'll submit new PR once I fix those test issues. (Testing both MIQ Redux specifics like addReducer function, as well as the overall Redux workflow using $ngRedux store impl.)

@karelhala @vojtechszocs I've seen a PR at theforeman/foreman#4925 that leverages https://github.com/randallknutson/redux-injector - might be useful here as well.

redux-injector allows to dynamically add ("inject") new reducers into the Redux store, but that store has to be created via createInjectStore instead of the vanilla createStore.

In our Redux infra, the addReducer function is actually our analogy to injectReducer function from redux-injector. The difference is that addReducer doesn't do state subtree scoping for given reducer, but as I wrote above, we could easily do that by extending its signature:

// this is what we have now
// reducer operates on the whole state
addReducer(myReducer);

// this is what we can add later if needed
addReducer(myReducer, 'foo.bar');

Note that the second example is a direct analogy to injectReducer('foo.bar', myReducer) from redux-injector project.

Additionally, in our Redux infra, we plan to leverage ng-redux so that Angular components can utilize $ngRedux.connect function to easily connect a component to Redux store.

ng-redux works by registering $ngRedux Angular provider [1] whose $get method (used to provide specific instance) uses vanilla createStore [2]. Unless ng-redux allows customizing the way Redux store is created, we can't use redux-injector together with ng-redux.

[1] https://github.com/angular-redux/ng-redux/blob/master/src/index.js
[2] https://github.com/angular-redux/ng-redux/blob/master/src/components/ngRedux.js#L80

[vojtechszocs]

Unless ng-redux allows customizing the way Redux store is created, we can't use redux-injector together with ng-redux.

Actually I might try doing a PR for ng-redux to allow customizing the way Redux store is created, in which case we could drop our addReducer in favor of redux-injector's injectReducer. However, redux-injector seems discontinued (only a handful of commits and releases, last update Nov 7, 2016) so going with our own, simple addReducer API and improve/replace later is what I'd suggest for now.

[vojtechszocs]

A minimal Redux infra code (with all TypeScript & JSDoc goodness) is already done, I'll submit new PR once I fix those test issues.

Here's the PR, sorry for the late update.

[himdel]

@karelhala I think we can close this one?