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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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, IMiQRenderCallback } 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: IMiQRenderCallback){}
}

/**
* 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

@vojtechszocs vojtechszocs Aug 31, 2017

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

@vojtechszocs vojtechszocs Aug 31, 2017

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?: IMiQRenderCallback) {
let newCmp = new ExtensibleComponent(name, api, render);
source.onNext({action: 'add', payload: newCmp});
return newCmp;
}

/**
* 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 : {})
.filter(component => component && component.name === cmpName)
Copy link
Contributor

@vojtechszocs vojtechszocs Sep 4, 2017

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

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;

/**
* 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.');
}
});
38 changes: 38 additions & 0 deletions app/javascript/extensible-components/lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
export type RenderCallback = (element: HTMLElement) => void;

export interface IExtensionComponent {
action: string;
payload: any;
}

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

export interface IMiQRenderCallback {
[propName: string]: (renderCallback: RenderCallback) => void;
}

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

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

@vojtechszocs vojtechszocs Sep 4, 2017

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');
79 changes: 79 additions & 0 deletions spec/javascripts/packs/extensible-componenets.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
describe('Extensible components', function() {
var htmlPartial = '<div>something</div>';
var cmp;
var mockApi = {
onSomeAction: jasmine.createSpy('onSomeAction', function() {}),
onSomeActionWithParams: jasmine.createSpy('onSomeActionWithParams', function(someParam){}),
};

var mockRender = {
addButton: jasmine.createSpy('addButton', function(callback) {}),
addButton2: function(callback) { callback(htmlPartial); }
};

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 someCallback = jasmine.createSpy('someCallback', function(element) {});
subscription.with(function(component) {
component.render.addButton(someCallback);
component.render.addButton2(someCallback);
});
expect(mockRender.addButton).toHaveBeenCalledWith(someCallback);
expect(someCallback).toHaveBeenCalledWith(htmlPartial);
});
});

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'