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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/javascript/custom-typings.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
declare var ManageIQ: any;
declare var Rx: any;
67 changes: 67 additions & 0 deletions app/javascript/extensible-components/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { IExtensionComponent, IMiQApiCallback } from './lib';

const source = new Rx.Subject();
const items = new Set();

/**
* Class for easy creation of extensible component.
*/
export class ExtensibleComponent {
public delete: () => void;
constructor(public name: string, public api: IMiQApiCallback, public render: IMiQApiCallback){}
}

/**
* Helper function to create new component.
* @param name string name of new component.
* @param api callback functions to change inner logic of component.
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

* @param render callback function to apply render functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

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'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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

*/
function addComponent(name: string, api?: IMiQApiCallback, render?: IMiQApiCallback) {
let newCmp = new ExtensibleComponent(name, api, render);
source.onNext({action: 'add', payload: newCmp});
return newCmp;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

/**
* Helper function to subscribe to extension items based on component's name.
* @param cmpName name of component we want to subscribe to.
* @return object which has with and unsubscribe property. With is for callback to use found component and delete to
* unsubscribe from rxjs subject.
*/
function subscribe(cmpName: string) {
let unsubscribe;
return {
with: (callback) => {
unsubscribe = source
.map(sourceAction => sourceAction.action === 'add' ? sourceAction.payload : {})
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.filter(component => component && component.name === cmpName)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

.subscribe(cmp => cmp && callback(cmp));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


ManageIQ.extensions.items.forEach((component) => {
component.name === cmpName && callback(component);
});
},
delete: () => unsubscribe && unsubscribe.dispose()
}
}

const extensions = {
addComponent,
subscribe,
get items() { return items; }
}

ManageIQ.extensions = ManageIQ.extensions || extensions;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@karelhala karelhala Sep 4, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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'

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 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.


/**
* Subscribe to extensions source to add new components to items object.
*/
source.subscribe((component: IExtensionComponent) => {
if (component.action === 'add' && component.hasOwnProperty('payload')) {
component.payload.delete = () => ManageIQ.extensions.items.delete(component.payload);
ManageIQ.extensions.items.add(component.payload);
} else {
console.error('Unsupported action with extension components.');
}
});
32 changes: 32 additions & 0 deletions app/javascript/extensible-components/lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export interface IExtensionComponent {
action: string;
payload: any;
}

export interface IMiQApiCallback {
[propName: string]: Function;
}

export interface IExtensibleComponent {
extensibleComponent: any;
apiCallbacks: () => IMiQApiCallback;
renderCallbacks: () => IMiQApiCallback;
}

export function getItems() {
return ManageIQ.extensions.items;
}

export function subscribe(cmpName: string) {
return ManageIQ.extensions.subscribe(cmpName);
}

/**
* Helper function to create new component.
* @param name string name of new component.
* @param api callback functions to change inner logic of component.
* @param render callback function to apply render functions.
*/
export function addComponent(name: string, api?: IMiQApiCallback, render?: IMiQApiCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, we could also do:

export const addComponent = ManageIQ.extensions.addComponent;

to cut down the boilerplate code. But I'm not sure how TypeScript fits the above syntax form 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, however I am not fan of assigning functions to const. So to be consistent we should all three things export as functions getItems subscribe addComponent

Copy link
Member

Choose a reason for hiding this comment

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

@karelhala just wondering - why you are not a fan?

Copy link
Contributor Author

@karelhala karelhala Sep 4, 2017

Choose a reason for hiding this comment

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

@ohadlevy well mostly due to readability. I really like fat arrows, but for named functions I prefer the "old" way instead of assign to const 😄

function getTen() {
   return 10;
}

Is somewhat more readable and from first glance you see that it's function, instead when reading this

const getTen = () => 10;

It looks nice and all, but the person reading this has to know that it's function assignment into const.

Plus binding is kinda strange with the second one.

function printA() {
  console.log(this.a);
}

const printA2 = () => console.log(this.a)

printA.bind({a: 'someText'})();
printA2.bind({a: 'someText'})();

The second one ignores binding of object, I know that it's the reason how this is handled with let and const, but that's not how named functions should behave. Again for anonymous functions it's the best things to do (no need to bind this to every function).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that lambdas declared through => are best used for anonymous functions.

From variable scoping point of view, const foo = function() {...} should be the same as function foo() {...} . Also, from TypeScript perspective:

const add1 = (a: number, b: number): number => { return a + b; };
function add2 (a: number, b: number): number { return a + b; };

The second one ignores binding of object

Right, lambdas don't have their own this and this should be taken into account when passing those around.

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, lambdas don't have their own this and this should be taken into account when passing those around.

Exactly, but

const getTen = () => 10;

typeof(getTen);

Will return function so one could assume it's "normal" function and call, bind, apply and such will behave differently (of course programmer would not call this inside lambda expecting to be set by someone else). But you get the picture why I prefer plain old function for named functions instead of assign to const

return ManageIQ.extensions.addComponent(name, api, render);
}
1 change: 1 addition & 0 deletions app/javascript/packs/extensible-componenets-common.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('../extensible-components');
75 changes: 75 additions & 0 deletions spec/javascripts/packs/extensible-componenets.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
describe('Extensible components', function() {
var cmp;
Copy link
Member

Choose a reason for hiding this comment

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

q. why var vs const, does it not support babel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are not taken care by webpack or anything, so that's why we can't use const, => and any ES6 goodies 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests should be treated with same love and care as production code.

Good testing tools do the JavaScript transpilation for you, so you don't have to mess with your existing webpack configuration. When using Jest, just add .babelrc file and Jest will use that.

@martinpovolny @karelhala BTW, Jest includes Jasmine as its test runner.

var mockApi = {
onSomeAction: jasmine.createSpy('onSomeAction', function() {}),
onSomeActionWithParams: jasmine.createSpy('onSomeActionWithParams', function(param) {}),
};

var mockRender = {
addButtonHtml: jasmine.createSpy('addButtonHtml', function(htmlElem) {})
};

it('should be defined with empty items', function() {
expect(ManageIQ.extensions).toBeDefined();
expect(ManageIQ.extensions.items.size).toBe(0);
});

it('should create one item', function() {
cmp = ManageIQ.extensions.addComponent('testCmp', mockApi, mockRender);
expect(ManageIQ.extensions.items.size).toBe(1);
});

it('should remove newly created item', function() {
cmp.delete();
expect(ManageIQ.extensions.items.size).toBe(0);
});

describe('default actions', function() {
var subscription;
var someParam = 'something';

beforeEach(function() {
cmp = ManageIQ.extensions.addComponent('testCmp', mockApi, mockRender);
});

describe('subscription', function() {
it('should subscribe based on name', function() {
subscription = ManageIQ.extensions.subscribe('testCmp');
expect(subscription.delete).toBeDefined();
expect(subscription.with).toBeDefined();
});

it('should react to API', function() {
subscription.with(function(component) {
component.api.onSomeAction();
component.api.onSomeActionWithParams(someParam);
});
expect(mockApi.onSomeAction).toHaveBeenCalled();
expect(mockApi.onSomeActionWithParams).toHaveBeenCalledWith(someParam);
});

it('should react to render', function() {
var someHTML = '<div>something</div>';
subscription.with(function(component) {
component.render.addButtonHtml(someHTML);
});
expect(mockRender.addButtonHtml).toHaveBeenCalledWith(someHTML);
});
});

it('should not subscribe', function() {
subscription = ManageIQ.extensions.subscribe('somethingBad');
subscription.with(function(component) {
//callback should not be called!
expect(false).toBe(true);
});
expect(subscription.with).toBeDefined();
expect(subscription.delete).toBeDefined();
});

afterEach(function() {
cmp.delete();
subscription && subscription.delete();
});
});
});
2 changes: 1 addition & 1 deletion spec/javascripts/support/jasmine.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ src_files:
- assets/angular-mocks
- __spec__/helpers/fixtures-fix.js
- packs/manageiq-ui-classic/application-common.js
- packs/manageiq-ui-classic/extensible-componenets-common.js

# stylesheets
#
Expand Down Expand Up @@ -126,4 +127,3 @@ boot_files:
#
# rack_options:
# server: 'thin'