-
Notifications
You must be signed in to change notification settings - Fork 356
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
Add new JS component API #3228
Conversation
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 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, |
This pull request is not mergeable. Please rebase and repush. |
2aeaddb
to
bf88c2a
Compare
bf88c2a
to
917ac5a
Compare
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. |
Following example illustrates the use of
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 // 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 But if the var breadcrumb = ManageIQ.component.newInstance( /* just like above */ );
breadcrumb.update({ items: newItems }); // or, breadcrumb.props.items = newItems;
breadcrumb.destroy(); |
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 |
@himdel @ZitaNemeckova +1 on Note that |
@vojtechszocs you'll have to test it.. |
@vojtechszocs Also looks like the js spec failure is real... jest tests probably fail because they are not configured for and jasmine specs fail because we need to mock Proxy (not possible as far as I know) or make sure that |
OK @vojtechszocs this looks really good, not seeing any problems at all, except for the use of 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 |
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", |
Well, the Angular app integration in "old" Jasmine specs works like this:
In app.run(['$ngRedux', ($ngRedux) => {
ManageIQ.redux = { .. };
}]); and in But I've just noticed that Zita's tests (the "new" ones with 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. |
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?
The "old" specs should work with webpack/er-processed JS output, e.g. |
OK, just checked that generated @himdel +1 on adding Edit: I could also re-write |
@himdel I'm working on the remaining items as outlined in your comment.
No problem, I can create separate PR for that. |
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) |
@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).
So... if there's a sane way of doing this without a |
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. |
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. I had to update Jest configuration, mocking the window.ManageIQ = {
angular: {
app: angular.module('ManageIQ', ['ngRedux'])
}
}; (Notice that we still need the @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. |
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 ( |
@himdel I don't have a strong opinion here, using |
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 In the button component example above, if the component's firstButton.props.foo = 'Foo Button'; then using ad-hoc proxies would actually break the API, causing the component's Because of that, I'll go with ES6 |
917ac5a
to
fbe6b5d
Compare
@himdel I've broken down this PR into following bits to make it easier to review:
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) |
@himdel @karelhala FYI, webpack build stats for the example 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:
with above component:
This shows two things:
|
To show that the new component API (consisting of 4 public methods) has really, really good test coverage:
|
This pull request is not mergeable. Please rebase and repush. |
Is it possible to retrigger CC & Travis without doing git push again? |
fbe6b5d
to
3c6714e
Compare
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 |
This pull request is not mergeable. Please rebase and repush. |
Add new JS component API (from #3228)
Closing as #3228 was merged. Thx! |
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:
Breadcrumb
to more complex components likeSerialConsole
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
(*) 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
frompatternfly-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
is used to define new components. Each component has a unique name, in this case it'sMiqButton
. 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:The
blueprint
function exists so that we don't have to writeReactDOM.render
and other boilerplate that is common to all React components. All we have to do is tell theblueprint
function howprops
map to React element and it takes care of the rest: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 ReactButton
component requires some content ("children") in order to look meaningful.Here, we're passing all props to
Button
while also providing the content based onprops.text
. This means our logicalMiqButton
component supports all props of underlying ReactButton
itself, plus a customtext
prop. When using theblueprint
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):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 withclass='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:
Each component instance has an
id
that is unique for the given component. Theid
can be provided by theblueprint.create
method, but is typically auto-generated as<Name>-<InstanceCount>
. Theid
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 toupdate
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!
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: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:
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 theblueprint
function howprops
map tointeract
property as the second argument:We can then call
firstButton.interact.say('Hello')
. Theinstance.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:When defining the
MiqNavMenu
component, we're using an empty object as the blueprint - this means callingManageIQ.component.newInstance
for this component will have no effect (it will returnundefined
).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:Other API methods
To check whether a component with the given name is already defined:
React + Redux ❤️
The
blueprint
function frommiq-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:
Then, define new component like so:
Summary
define
,newInstance
,getInstance
,isDefined