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 new JS component API #3228

Closed
wants to merge 3 commits into from

Conversation

vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented Jan 11, 2018

JavaScript component API

This PR supports the vision where different web UI technologies co-exist under a minimal API.

Motivation

The new API allows component reuse & interaction regardless of the underlying UI technology, be it React, Angular or anything else. DOM and JavaScript is the baseline. The main idea is to allow rendering a logical component into the given DOM element and to be able to interact with that component.

This PR is meant to support following use cases:

  1. use React-based component inside Angular component or directly in HAML template, ranging from simple presentational components like Breadcrumb to more complex components like SerialConsole
  2. use Angular-based component inside React component, e.g. GTL component used in plugin-contributed, React-based Rails view
  3. allow interaction with existing component instances, e.g. adding new items to the main menu on the left (this can be done today with Ruby Menu::CustomLoader)

Common React components can be shared and consumed via patternfly-react repo. ManageIQ specific components are already consumed via ManageIQ/ui-components repo.

What is covered

  • component API & implementation with accompanying unit tests (*)
  • detailed TypeScript definition of the component API, describing its methods and contracts
  • support for React, including seamless integration with ManageIQ Redux API

(*) With Jest now supported, I'll rewrite both Redux and component API tests to use Jest in a follow-up PR. In case of the Redux API, we'll first need to figure out how to emulate beforeEach(module('ManageIQ')) due to Angular-specific dependency on $ngRedux.

What is not covered (yet)

API walk through

Let's say we want to plug Button from patternfly-react into the component API and use it in the "About" dialog.

Defining components

The first step is to modify app/javascript/miq-component-react/index.tsx that contains all React-based component definitions:

ManageIQ.component.define('MiqButton', blueprint(
  (props) => (
    <Button {...props}>
      {props.text || '(no text specified)'}
    </Button>
  )
));

ManageIQ.component.define is used to define new components. Each component has a unique name, in this case it's MiqButton. Once defined, we can refer to that component by its name.

The blueprint function helps us to create a blueprint for our component. Think of a blueprint as a recipe for creating, updating and destroying instances of the given component:

const exampleBlueprint = {

  create(props, mountTo) {
    // create and mount component instance
  },

  update(newProps, mountedTo) {
    // update component instance
  },

  destroy(instance, unmountFrom) {
    // destroy and unmount component instance
  }

};

The blueprint function exists so that we don't have to write ReactDOM.render and other boilerplate that is common to all React components. All we have to do is tell the blueprint function how props map to React element and it takes care of the rest:

(props) => (
  <Button {...props}>
    {props.text || '(no text specified)'}
  </Button>
)

Props are component's inputs, containing data and callbacks. This term is taken from React - it's just a fancy name for an object holding all the inputs relevant for the component.

Notice that we could also simply return (props) => <Button {...props} />. We don't do that since the React Button component requires some content ("children") in order to look meaningful.

Here, we're passing all props to Button while also providing the content based on props.text. This means our logical MiqButton component supports all props of underlying React Button itself, plus a custom text prop. When using the blueprint function, you're free to return whatever React component representation suits you best.

Creating component instances

Let's use our newly defined component. Open app/views/layouts/_about_modal.html.haml and put in the following code (translate to ES5 if needed):

const firstButton = ManageIQ.component.newInstance(
  'MiqButton',
  {
    block: true,
    text: 'Test Button',
    onClick: () => { console.log('clicked button', firstButton.id); }
  },
  $('.test-button').get(0)
);

ManageIQ.component.newInstance creates a new instance of the given component and mounts (renders) it into a DOM element. We're passing initial props as the second argument and the target DOM element as the third argument (assuming there's one with class='test-button' into which we can render).

We get back an object called "component instance", representing the underlying technology specific (React) component. Component instances can be interacted with:

console.log(firstButton.id);
// MiqButton-0

console.log(firstButton.props);
// { block: true, text: 'Test Button', onClick: <Fn> }

firstButton.update({ text: 'Updated Button', bsStyle: 'primary' });
console.log(firstButton.props);
// { block: true, text: 'Updated Button', bsStyle: 'primary', onClick: <Fn> }

Each component instance has an id that is unique for the given component. The id can be provided by the blueprint.create method, but is typically auto-generated as <Name>-<InstanceCount>. The id exists so that you can distinguish between individual instances of the same component.

What does instance.update actually do? It merges current props with ones passed to update method, creating new object that will become the next current props. Then, it tells the blueprint to update the component instance, e.g. re-render it based on the current props.

Modern UI technologies endorse one-way data binding, which means the props object is treated as immutable. This greatly simplifies the data flow in your application, making it easier to understand and change.

That said, the component API also supports direct props modification. Don't do this unless you absolutely have to!

// multiple props modifications done in the same event loop tick ...
firstButton.props.text = 'Foo Button';
firstButton.props.block = false;

// ... will cause the following to happen in the next event loop tick:
firstButton.update({ text: 'Foo Button', block: false });

Destryoing component instances

When you're done with the given instance, just call the instance.destroy method to destroy and unmount it from the associated DOM element:

firstButton.destroy();
// at this point, accessing anything other than firstButton.id will throw an error

Make sure to destroy component instances once they're no longer needed to prevent memory leaks!

Interacting with instances

There are two types of interaction, based on who initiates it:

  1. initiated by the component instance
  2. initiated by the code that manages the component instance

The first type is commonly referred to as "callbacks". It's represented through functions in component's props.

The second type is implemented through the instance.interact property, whose value is entirely component specific (and optional). When defining new component, just tell the blueprint function how props map to interact property as the second argument:

ManageIQ.component.define('MiqButton', blueprint(
  (props) => (
    <Button {...props}>
      {props.text || '(no text specified)'}
    </Button>
  ),
  (props) => ({
    say: (msg) => { console.log('MiqButton says:', msg); }
  })
));

We can then call firstButton.interact.say('Hello'). The instance.interact property is meant to be a component specific API, exposing public methods for working with that component.

Defining components with existing instances

A common use case is to have a logical component that doesn't need to be reused (instantiated) but needs to provide interaction with its instance(s). Let's say we have a React NavMenu component that has one existing instance as part of the application's layout:

const navMenuInstance = {
  id: 'the-one-and-only',
  interact: {
    addItem: (newItem) => { /* delegate to underlying React component */ }
  }
};

ManageIQ.component.define('MiqNavMenu', {}, [navMenuInstance]);

When defining the MiqNavMenu component, we're using an empty object as the blueprint - this means calling ManageIQ.component.newInstance for this component will have no effect (it will return undefined).

Notice that we're passing an array of existing instances as the third argument. This will associate those instances with our component. In this case, it's recommended to specify an explicit instance id for easier lookup.

Looking up component instances

Following the above MiqNavMenu example, here's how we can obtain the component instance so that we can interact with it:

const navMenuInstance = ManageIQ.component.getInstance(
  'MiqNavMenu',
  'the-one-and-only'
);

navMenuInstance.interact.addItem({ title: 'Hello', link: '#foo' });

Other API methods

To check whether a component with the given name is already defined:

if (ManageIQ.component.isDefined('MiqNavMenu')) {
  console.log('Defined and ready for instantiation and/or instance lookup');
}

React + Redux ❤️

The blueprint function from miq-component-react pack seamlessly integrates with ManageIQ Redux API. It wraps the given React element in <Provider store={ManageIQ.redux.store}> so that your connected React component hooks to ManageIQ Redux store as you'd expect.

To illustrate this, let's add an example reducer and initialize the relevant state subtree:

ManageIQ.redux.addReducer((state, action) => {
  if (action.type === 'REDUX_BUTTON_RESET') {
    var newState = Object.assign({}, state);
    newState.ReduxButton = { clicks: 0 };
    return newState;
  } else if (action.type === 'REDUX_BUTTON_CLICK') {
    var newState = Object.assign({}, state);
    newState.ReduxButton.clicks++;
    return newState;
  } else {
    return state;
  }
});

ManageIQ.redux.store.dispatch({ type: 'REDUX_BUTTON_RESET' });

Then, define new component like so:

const MiqButton = (props) => (
  <Button {...props}>
    {props.text || '(no text specified)'}
  </Button>
);

const MiqReduxButton = connect(
  (state) => ({
    text: `Redux Button clicks: ${state.ReduxButton.clicks}`
  }),
  (dispatch) => ({
    onClick: () => { dispatch({ type: 'REDUX_BUTTON_CLICK' }); }
  })
)(MiqButton);

ManageIQ.component.define('MiqReduxButton', blueprint(
  (props) => <MiqReduxButton {...props} />
));

Summary

  • minimal API: define, newInstance, getInstance, isDefined
  • UI technology agnostic, DOM and JavaScript is the baseline
  • can interact with component instances and destroy them when no longer needed
  • can specify existing instances when defining new component
  • React and Redux support

@vojtechszocs
Copy link
Contributor Author

@vojtechszocs
Copy link
Contributor Author

vojtechszocs commented Jan 11, 2018

This PR generalizes Ohad's commit for React support at ohadlevy/manageiq-ui-classic@b6455cf

Edit: following information is outdated, please read the PR description for detailed overview of the new API.

We'll still want to add react and react-dom dependencies to package.json to make the definition of React-based components as simple as possible, using a blueprint helper:

const ExampleButton = ({ foo, onBar }) => (
  <button onClick={onBar}>{foo}</button>
);

ManageIQ.component.define(
  'ExampleButton',
  getReactBlueprint(ExampleButton) // blueprint helper
);

As for Angular 1.5+ based components, it should look like:

const ExampleButton = {
  bindings: { // prefer one-way bindings
    foo: '<',
    onBar: '<'
  },
  template: `
    <button ng-click='$ctrl.onBar($event)'>{$ctrl.foo}</button>
  `
}

ManageIQ.angular.app.component('exampleButton', ExampleButton);

ManageIQ.component.define(
  'ExampleButton',
  getAngularBlueprint(ExampleButton) // blueprint helper
);

In both cases, ExampleButton is the UI technology specific component representation - we don't need to implement blueprint.create and blueprint.destroy methods, the helper does that for us.

@mzazrivec mzazrivec added the wip label Jan 11, 2018
@himdel himdel changed the title Add new JS component API [WIP] Add new JS component API Jan 11, 2018
@ohadlevy ohadlevy mentioned this pull request Jan 21, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 29, 2018

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

@vojtechszocs
Copy link
Contributor Author

vojtechszocs commented Feb 9, 2018

@jelkosz @ohadlevy @himdel I've updated PR description to walk through all parts of the new component API, let me know what you think.

This PR also includes some smaller improvements of the Redux API.

@vojtechszocs vojtechszocs changed the title [WIP] Add new JS component API Add new JS component API Feb 9, 2018
@vojtechszocs
Copy link
Contributor Author

Resetting PR title since it's no longer a WIP, it's ready and working.

The PR doesn't impact existing functionality, it just adds the new component mechanism.

@miq-bot miq-bot removed the wip label Feb 9, 2018
@vojtechszocs
Copy link
Contributor Author

Following example illustrates the use of Breadcrumb component.

app/javascript/miq-component-react/index.tsx

import { Breadcrumb, BreadcrumbItem } from 'patternfly-react';

ManageIQ.component.define('Breadcrumb', blueprint(
  (props) => (
    <Breadcrumb>
      {props.items.map((item, index) => (
        <BreadcrumbItem
          key={item.id || index}
          href={item.href || '#'}
          active={index === props.items.length - 1}>
          {item.text}
        </BreadcrumbItem>
      ))}
    </Breadcrumb>
  )
));

ES5 script in app/views/layouts/_breadcrumbs.html.haml

// should be derived from the relevant Ruby object representation
var items = [
  { href='#example', text: 'Home' },
  { text: 'You are here' }
];

ManageIQ.component.newInstance(
  'Breadcrumb',
  { items: items }, 
  document.querySelector('.container')
);

In this case, the Breadcrumb component instance lives "forever" (unless you navigate to a different web page) so we don't need to reference & destroy it. Also, the items are static so we don't need to interact with the instance.

But if the Breadcrumb component instance was short-lived or had to be dynamically updated:

var breadcrumb = ManageIQ.component.newInstance( /* just like above */ );
breadcrumb.update({ items: newItems }); // or, breadcrumb.props.items = newItems;
breadcrumb.destroy();

@himdel
Copy link
Contributor

himdel commented Feb 19, 2018

In case of the Redux API, we'll first need to figure out how to emulate beforeEach(module('ManageIQ'))

Just a heads up, this one should be solved (at least in a "this works" way, not "this is pretty" way) - https://github.com/ManageIQ/manageiq-ui-classic/pull/3281/files#diff-dcaedc20b5520d497e018bc39b17b6bd

@vojtechszocs
Copy link
Contributor Author

@himdel @ZitaNemeckova +1 on mocks.js that stubs Angular stuff.

Note that app/javascript/miq-redux/index.ts uses the app.run hook, so ManageIQ.angular.app would have to be bootstrapped as well, right?

@himdel
Copy link
Contributor

himdel commented Feb 19, 2018

Note that app/javascript/miq-redux/index.ts uses the app.run hook, so ManageIQ.angular.app would have to be bootstrapped as well, right?

@vojtechszocs you'll have to test it.. ManageIQ.angular.app is mocked there and we never have to call miq_bootstrap in the old angular specs... so I think you shoulndn't need to do anything except to require('miq-redux/index').. but YMMV :)

@himdel
Copy link
Contributor

himdel commented Feb 22, 2018

@vojtechszocs Also looks like the js spec failure is real...

jest tests probably fail because they are not configured for *.spec.js (only *.test.js so far) (mentioned in https://github.com/ManageIQ/manageiq-ui-classic/pull/3440/files/9c9642934985bcddcd32e24cf0c5ed3f5524658a#discussion_r169948900, so this may change)

and jasmine specs fail because we need to mock Proxy (not possible as far as I know) or make sure that miq-component/utils.ts won't get loaded by the old specs..

@himdel
Copy link
Contributor

himdel commented Feb 28, 2018

OK @vojtechszocs this looks really good, not seeing any problems at all, except for the use of Proxy: it's not supported at all in IE.

So, I guess what we need to do:

It would also be nice if you could undo the non-changes (or put them in a separate commit at least) - all the changes in app/javascript/miq-redux/reducer.ts, app/javascript/miq-redux/index.ts, app/javascript/miq-redux/redux-typings.ts, app/javascript/miq-redux/store.ts, config/webpack/loaders/react.js and most of app/javascript/packs/application-common.js don't actually do anything except change whitespace or move stuff around ... not something you want in the same commit. (But, I'd consider this a "would be nice".)

@himdel
Copy link
Contributor

himdel commented Feb 28, 2018

Looks like this enables the proxy polyfill .. no longer complains about Proxy at least :).

diff --git a/app/javascript/packs/application-common.js b/app/javascript/packs/application-common.js
index 2da510bf2..53bffcd79 100644
--- a/app/javascript/packs/application-common.js
+++ b/app/javascript/packs/application-common.js
@@ -10,6 +10,8 @@ import miqRedux from 'miq-redux';
 import miqComponent from 'miq-component';
 import miqComponentReact from 'miq-component-react';
 
+require('proxy-polyfill');
+
 const app = ManageIQ.angular.app;
 
 // TODO(vs) link to article at http://talk.manageiq.org/c/developers
diff --git a/package.json b/package.json
index 4700c000d..0e2dec287 100644
--- a/package.json
+++ b/package.json
@@ -36,6 +36,7 @@
     "ng-redux": "^3.5.2",
     "patternfly-react": "^0.26.0",
     "prop-types": "^15.6.0",
+    "proxy-polyfill": "^0.1.7",
     "react": "^16.2.0",
     "react-dom": "^16.2.0",
     "react-redux": "^5.0.6",

@vojtechszocs
Copy link
Contributor Author

@himdel

we never have to call miq_bootstrap in the old angular specs...

Well, the Angular app integration in "old" Jasmine specs works like this:

  1. beforeEach(module('ManageIQ')) specifies what modules to use when creating the $injector (docs here). Note that ng and ngMock are automatically added to that module list.
  2. beforeEach(inject(function (/* inject stuff */) { .. })) creates the $injector and allows injecting stuff needed by the test (docs here). We're using beforeEach so each test case gets a brand new $injector instance.

In app/javascript/miq-redux/index.ts we're initializing the ManageIQ.redux namespace via the app module's run hook

app.run(['$ngRedux', ($ngRedux) => {
  ManageIQ.redux = { .. };
}]);

and in spec/javascripts/packs/miq-redux.spec.js we're testing if such namespace is defined. Since the test passes, the above mentioned workflow does include the execution of module's run hook. And, if I understand correctly, execution of the module's run hook is part of Angular app bootstrap process.

But I've just noticed that Zita's tests (the "new" ones with .test.js suffix) still use the above mentioned workflow, most importantly beforeEach(inject( .. )) which means the "old" tests can be ported to ES6 without issues.

For some reason, I thought we're going to avoid using ngMock in "new" tests, which is why I posted my original question 😃 sorry for confusion.

@vojtechszocs
Copy link
Contributor Author

jest tests probably fail because they are not configured for *.spec.js (only *.test.js so far)

AFAIK, the UI codebase is split into "old" JS (managed by Rails asset pipeline) vs. the "new" JS (managed by webpack/er) and this results in the split into "old" tests (Jasmine/PhantomJS) vs. the "new" tests (Jest). Did this change recently? Are you going to migrate all "old" tests into "new" tests, i.e. test everything through Jest?

and jasmine specs fail because we need to mock Proxy (not possible as far as I know) or make sure that miq-component/utils.ts won't get loaded by the old specs..

The "old" specs should work with webpack/er-processed JS output, e.g. spec/manageiq/public/packs/manageiq-ui-classic/application-common.js which should polyfill stuff like Proxy if needed. Since we're using TypeScript, I thought tsc takes care of that? 😕

@vojtechszocs
Copy link
Contributor Author

vojtechszocs commented Feb 28, 2018

OK, just checked that generated application-common.js doesn't polyfill Proxy which is probably due to the fact that most browsers already support it.

@himdel +1 on adding Proxy polyfill to avoid test breakage on old'ish browsers.

Edit: I could also re-write writeProxy function in app/javascript/miq-component/utils.ts to use ad-hoc proxy approach by replacing object properties with setters, if really necessary. (But since Proxy is the standard way to go, I went with it.)

@vojtechszocs
Copy link
Contributor Author

@himdel I'm working on the remaining items as outlined in your comment.

It would also be nice if you could undo the non-changes (or put them in a separate commit at least)

No problem, I can create separate PR for that.

@vojtechszocs
Copy link
Contributor Author

Fun facts:

Babel docs: Due to the limitations of ES5, Proxies cannot be transpiled or polyfilled.

MS support dates: Internet Explorer 11 support ends October 14, 2025. (based on extended support of Windows 10)

@himdel
Copy link
Contributor

himdel commented Mar 1, 2018

@vojtechszocs Yeah, proxies can't really be polyfilled but that one polyfill does exist and looks like it might support our use case... But we'd have to test it in IE, yes.

If not, there's yet another alternative - https://github.com/krzkaczor/babel-plugin-proxy .. but that one would probably mean having to do separate JS builds for IE (because changing every property access everywhere to a function call is not exactly cheap or something you want to do in general).

Edit: I could also re-write writeProxy function in app/javascript/miq-component/utils.ts to use ad-hoc proxy approach by replacing object properties with setters, if really necessary. (But since Proxy is the standard way to go, I went with it.)

So... if there's a sane way of doing this without a Proxy, maybe it'd be the best.
But OTOH if Proxy feels like the right approach and the alternative would be something really hacky, then maybe keeping Proxy and testing the polyfill would still be the best bet.

@himdel
Copy link
Contributor

himdel commented Mar 1, 2018

jest tests probably fail because they are not configured for *.spec.js (only *.test.js so far)

AFAIK, the UI codebase is split into "old" JS (managed by Rails asset pipeline) vs. the "new" JS (managed by webpack/er) and this results in the split into "old" tests (Jasmine/PhantomJS) vs. the "new" tests (Jest). Did this change recently? Are you going to migrate all "old" tests into "new" tests, i.e. test everything through Jest?

There's still a split (with the exception that application-common is included in old specs), sorry, I originally misunderstood :). I thought those were supposed to be jest specs, not jasmine. Looks like the current jest failure is caused simply by the removal of the only spec that was there :).

Seems sane that jest would fail if there's no specs, so... if you're moving the specs to jest, this will solve itself, and if not, you can just add a trivial passing spec to make jest not complain.

@vojtechszocs
Copy link
Contributor Author

I've managed to port both Redux and Component API tests over to Jest & jsdom test environment, resolving all discrepancies between Jasmine and Jest APIs.

@ohadlevy I've also reduced reliance on Angular stuff, e.g. miq-component pack has no Angular dependencies now.

I had to update Jest configuration, mocking the ManageIQ global like so:

window.ManageIQ = {
  angular: {
    app: angular.module('ManageIQ', ['ngRedux'])
  }
};

(Notice that we still need the ngRedux module, since we're testing the actual $ngRedux implementation which backs our Redux API.)

@ZitaNemeckova regarding access to underlying React element, this can be part of React support code specifically, as it doesn't really fit the notion of a UI technology agnostic mechanism. Here is my proposal, let me know what you think:

const button = .. // created through ManageIQ.component.newInstance
const reactButtonElement = button.reactElement(); // assuming button is React-based

I'll split this PR into multiple PRs for better review.

@vojtechszocs
Copy link
Contributor Author

Seems sane that jest would fail if there's no specs, so... if you're moving the specs to jest, this will solve itself, and if not, you can just add a trivial passing spec to make jest not complain.

Jest auto-fails if it doesn't find any tests to execute. I've ported Redux and Component API tests over to Jest so it's not a problem anymore. We should encourage a policy that "new" JS code (app/javascript) should have its tests executed by Jest, promoting test code quality and ideally co-locating test code next to production code.

@vojtechszocs
Copy link
Contributor Author

So... if there's a sane way of doing this without a Proxy, maybe it'd be the best.
But OTOH if Proxy feels like the right approach and the alternative would be something really hacky, then maybe keeping Proxy and testing the polyfill would still be the best bet.

writeProxy helper in app/javascript/miq-component/utils.ts is used to intercept writes to properties of the given object. It can be re-implemented by replacing all own enumerable properties of the given object with setters, which should also be the case of Proxy ES5 polyfill for the set trap.

@himdel I don't have a strong opinion here, using Proxy is simpler but it requires polyfill (for older browsers and IE ❄️) and the writeProxy use case is quite simple. I'll probably re-write the method not to use Proxy to avoid pulling in the polyfill.

@vojtechszocs
Copy link
Contributor Author

I'll probably re-write the method not to use Proxy to avoid pulling in the polyfill.

Unfortunately, with ad-hoc proxies containing accessor methods (getters and setters) it is not possible to cover properties that are added in the future. See my JS Bin example that compares ES6 Proxy vs. ad-hoc proxy implementations.

In the button component example above, if the component's props doesn't contain foo property and we do

firstButton.props.foo = 'Foo Button';

then using ad-hoc proxies would actually break the API, causing the component's instance.update method not to be called.

Because of that, I'll go with ES6 Proxy and use proxy-polyfill as suggested by @himdel.

@vojtechszocs
Copy link
Contributor Author

@himdel I've broken down this PR into following bits to make it easier to review:

Proxy is polyfilled by importing proxy-polyfill in the application-common pack. Tests are migrated to Jest. Example MiqButton component was removed. Ability to get actual React element can be done in a follow-up PR.

Please review and let me know what you think.

Merge dependencies: #3517, #3518. (there are 3 commits total, 2 for dependant PRs and one for this PR)

@vojtechszocs
Copy link
Contributor Author

@himdel @karelhala FYI, webpack build stats for the example MiqButton component:

import { Button } from 'patternfly-react'; // pulls in lots of stuff

ManageIQ.component.define('MiqButton', blueprint(
  (props) => (
    <Button {...props}>
      {props.text || '(no text specified)'}
    </Button>
  )
));

without above component:

                                    Asset       Size  Chunks             Chunk Names
manageiq-ui-classic/application-common.js      46 kB       0  [emitted]  manageiq-ui-classic/application-common
                                vendor.js     149 kB       1  [emitted]  vendor
                            manifest.json  136 bytes          [emitted]  

with above component:

                                    Asset       Size  Chunks                    Chunk Names
manageiq-ui-classic/application-common.js    52.9 kB       0  [emitted]         manageiq-ui-classic/application-common
                                vendor.js    6.45 MB       1  [emitted]  [big]  vendor
                            manifest.json  136 bytes          [emitted]         

This shows two things:

  1. vendor chunk seems to work as expected 😃
  2. patternfly-react pulls in lots of stuff, but I guess that's expected (lots of transitive dependencies)

@vojtechszocs
Copy link
Contributor Author

To show that the new component API (consisting of 4 public methods) has really, really good test coverage:

 PASS  app/javascript/miq-component/test.js
  Component API
    ✓ ManageIQ.component namespace is defined (3ms)
    ✓ define method can be used to register new components (1ms)
    ✓ define method does nothing if the component name is already taken (1ms)
    ✓ define method does nothing if the component name is not a string
    ✓ define method can be used associate existing instances with the new component (1ms)
    ✓ when passing existing instances, define method ensures a sane instance id (1ms)
    ✓ when passing existing instances, define method ensures that instance id is frozen (1ms)
    ✓ when passing existing instances, define method skips falsy values
    ✓ when passing existing instances, define method throws in case of reference duplicity (1ms)
    ✓ when passing existing instances, define method throws in case of id duplicity (1ms)
    ✓ newInstance method can be used to create new component instances (2ms)
    ✓ newInstance method does nothing if the component is not already defined (1ms)
    ✓ newInstance method does nothing if blueprint.create is not a function
    ✓ newInstance method throws if blueprint.create returns a falsy value (1ms)
    ✓ newInstance method ensures a sane instance id (2ms)
    ✓ newInstance method ensures that instance id is frozen (3ms)
    ✓ newInstance method throws if blueprint.create returns the same instance (1ms)
    ✓ newInstance method throws if blueprint.create returns an instance with id already taken (1ms)
    ✓ trying to rewrite instance props throws an error
    ✓ instance.update merges props and delegates to blueprint.update (1ms)
    ✓ instance.update does nothing if blueprint.update is not a function (1ms)
    ✓ multiple props modifications will trigger single instance.update (4ms)
    ✓ instance.destroy delegates to blueprint.destroy (2ms)
    ✓ instance.destroy removes the component instance (1ms)
    ✓ instance.destroy prevents access to existing instance properties except for id (1ms)
    ✓ getInstance method can be used to obtain existing component instances (1ms)
    ✓ isDefined method can be used to determine whether a component is already defined
    ✓ getDefinition returns the correct component definition
    ✓ sanitizeAndFreezeInstanceId ensures the instance id is sane and cannot be changed (1ms)
    ✓ validateInstance performs the necessary instance validations
    ✓ writeProxy can be used to proxy writes to properties of the given object (1ms)
    ✓ lockInstanceProperties prevents access to existing instance properties except for id (1ms)

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2018

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

@vojtechszocs
Copy link
Contributor Author

Is it possible to retrigger CC & Travis without doing git push again?

@vojtechszocs vojtechszocs reopened this Mar 7, 2018
@miq-bot
Copy link
Member

miq-bot commented Mar 8, 2018

Checked commits vojtechszocs/manageiq-ui-classic@a136d06~...3c6714e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2018

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

martinpovolny added a commit that referenced this pull request Mar 21, 2018
Add new JS component API (from #3228)
@martinpovolny
Copy link
Member

Closing as #3228 was merged. Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants