Merge pull request #7070 from himdel/react-override

React - override id, component; convert Breadcrumbs to functional

(cherry picked from commit d4a1b0d)

app/helpers/reactjs_helper.rb

@@ -1,6 +1,6 @@
 module ReactjsHelper
   def react(name, data = {}, attributes = {}, element = 'div')
-    uid = unique_html_id('react')
+    uid = attributes[:id] || unique_html_id('react')
     capture do
       concat(content_tag(element, nil, attributes.merge(:id => uid)))
       concat(javascript_tag("ManageIQ.component.componentFactory('#{j(name)}', '##{j(uid)}', #{data.to_json})"))

app/javascript/components/breadcrumbs/index.jsx

@@ -1,81 +1,69 @@
-import React, { Component } from 'react';
+import React from 'react';
 import PropTypes from 'prop-types';
 import { Breadcrumb } from 'patternfly-react';
 import { unescape } from 'lodash';
 
 import { onClickTree, onClick, onClickToExplorer } from './on-click-functions';
 
+// FIXME: don't parse html here
 const parsedText = text => unescape(text).replace(/<[/]{0,1}strong>/g, '');
 
-class Breadcrumbs extends Component {
-  renderItems = () => {
-    const { items, controllerName } = this.props;
-    return items.filter((_item, index) => index !== (items.length - 1)).map((item, index) => {
+const renderItems = ({ items, controllerName }) => {
+  return items
+    .filter((_item, index) => index !== (items.length - 1))
+    .map((item, index) => {
       const text = parsedText(item.title);
-      if ((item.url || item.key || item.to_explorer) && !item.action) {
-        if (item.key || item.to_explorer) {
-          return (
-            <Breadcrumb.Item
-              key={`${item.key}-${index}`} // eslint-disable-line react/no-array-index-key
-              onClick={e =>
-                (item.to_explorer
-                  ? onClickToExplorer(e, controllerName, item.to_explorer)
-                  : onClickTree(e, controllerName, item))
-                }
-            >
-              {text}
-            </Breadcrumb.Item>
-          );
-        }
+      if (item.action || (!item.url && !item.key && !item.to_explorer)) {
+        return <li key={index}>{text}</li>; // eslint-disable-line react/no-array-index-key
+      }
+
+      if (item.key || item.to_explorer) {
         return (
           <Breadcrumb.Item
-            key={item.url || index}
-            href={item.url}
-            onClick={e => onClick(e, item.url)}
+            key={`${item.key}-${index}`} // eslint-disable-line react/no-array-index-key
+            onClick={e =>
+              (item.to_explorer
+                ? onClickToExplorer(e, controllerName, item.to_explorer)
+                : onClickTree(e, controllerName, item))
+              }
           >
             {text}
           </Breadcrumb.Item>
         );
       }
-      return <li key={index}>{text}</li>; // eslint-disable-line react/no-array-index-key
-    });
-  };
 
-  render() {
-    const {
-      items, title, controllerName, ...rest // eslint-disable-line no-unused-vars
-    } = this.props;
-
-    return (
-      <Breadcrumb style={{ marginBottom: 0 }} {...rest}>
-        {items && this.renderItems()}
-        <Breadcrumb.Item active>
-          <strong>
-            {items && items.length > 0 ? parsedText(items[items.length - 1].title) : parsedText(title)}
-          </strong>
+      return (
+        <Breadcrumb.Item
+          key={item.url || index}
+          href={item.url}
+          onClick={e => onClick(e, item.url)}
+        >
+          {text}
         </Breadcrumb.Item>
-      </Breadcrumb>
-    );
-  }
-}
+      );
+    });
+};
+
+const Breadcrumbs = ({ items, title, controllerName }) => (
+  <Breadcrumb style={{ marginBottom: 0 }}>
+    {items && renderItems({ items, controllerName })}
+    <Breadcrumb.Item active>
+      <strong>
+        {items && items.length > 0 ? parsedText(items[items.length - 1].title) : parsedText(title)}
+      </strong>
+    </Breadcrumb.Item>
+  </Breadcrumb>
+);
 
 Breadcrumbs.propTypes = {
-  items: PropTypes.arrayOf(PropTypes.shape(
-    {
-      title: PropTypes.string.isRequired,
-      url: PropTypes.string,
-      action: PropTypes.string,
-      key: PropTypes.string,
-    },
-  )),
-  title: PropTypes.string,
   controllerName: PropTypes.string,
-};
-
-Breadcrumbs.defaultProps = {
-  items: undefined,
-  title: undefined,
-  controllerName: undefined,
+  items: PropTypes.arrayOf(PropTypes.shape({
+    action: PropTypes.string,
+    key: PropTypes.string,
+    title: PropTypes.string.isRequired,
+    url: PropTypes.string,
+  })),
+  title: PropTypes.string,
 };
 
 export default Breadcrumbs;

app/javascript/miq-component/helpers.js

@@ -1,8 +1,8 @@
 import * as registry from './registry.js';
 import reactBlueprint from './react-blueprint.jsx';
 
-export function addReact(name, component) {
-  return registry.define(name, reactBlueprint(component));
+export function addReact(name, component, options = {}) {
+  return registry.define(name, reactBlueprint(component), options);
 }
 
 export function componentFactory(blueprintName, selector, props) {

app/javascript/miq-component/registry.js

@@ -1,29 +1,27 @@
 import { writeProxy, lockInstanceProperties } from './utils';
 import { cleanVirtualDom } from './helpers';
 
-const registry = new Map(); // Map<definition, Set<instance>>
+const registry = {}; // Map<name, {name, blueprint, instances: Set}>
 
 /**
  * Get definition of a component with the given `name`.
  */
 export function getDefinition(name) {
-  return Array.from(registry.keys()).find(definition => definition.name === name);
+  return registry[name];
 }
 
 /**
  * Make sure the instance `id` is sane and cannot be changed.
  */
 export function sanitizeAndFreezeInstanceId(instance, definition) {
-  const id = typeof instance.id === 'string'
-    ? instance.id
-    : `${definition.name}-${registry.get(definition).size}`;
+  const id = instance.id || `${definition.name}-${definition.instances.size}`;
 
   Object.defineProperty(instance, 'id', {
     get() {
       return id;
     },
     set() {
-      throw new Error(`Attempt to modify id of instance ${instance.id}`);
+      throw new Error(`Attempt to modify id of instance ${id}`);
     },
     enumerable: true,
   });
@@ -35,7 +33,7 @@ export function sanitizeAndFreezeInstanceId(instance, definition) {
  * - the given instance `id` isn't already taken
  */
 export function validateInstance(instance, definition) {
-  if (Array.from(registry.get(definition)).find(existingInstance => existingInstance === instance)) {
+  if (Array.from(definition.instances).find(existingInstance => existingInstance === instance)) {
     throw new Error('Instance already present, check your blueprint.create implementation');
   }
   if (getInstance(definition.name, instance.id)) {
@@ -46,23 +44,28 @@ export function validateInstance(instance, definition) {
 /**
  * Implementation of the `ComponentApi.define` method.
  */
-export function define(name, blueprint = {}, instances = null) {
+export function define(name, blueprint = {}, options = {}) {
   // validate inputs
-  if (typeof name !== 'string' || isDefined(name)) {
-    return;
+  if (typeof name !== 'string') {
+    throw new Error(`Registry.define: non-string name: ${name}`);
+  }
+  if (isDefined(name) && !options.override) {
+    throw new Error(`Registry.define: component already exists: ${name} (use { override: true } ?)`);
   }
 
   // add new definition to the registry
-  const newDefinition = { name, blueprint };
-  registry.set(newDefinition, new Set());
+  const instances = new Set();
+  const newDefinition = { name, blueprint, instances };
+  registry[name] = newDefinition;
 
   // add existing instances to the registry
-  if (Array.isArray(instances)) {
-    instances.filter((instance) => !!instance)
+  if (Array.isArray(options.instances)) {
+    options.instances.filter((instance) => !!instance)
       .forEach((instance) => {
         sanitizeAndFreezeInstanceId(instance, newDefinition);
         validateInstance(instance, newDefinition);
-        registry.get(newDefinition).add(instance);
+
+        newDefinition.instances.add(instance);
       });
   }
 }
@@ -147,7 +150,7 @@ export function newInstance(name, initialProps = {}, mountTo = undefined) {
     }
 
     // remove instance from the registry
-    registry.get(definition).delete(newInstance);
+    definition.instances.delete(newInstance);
 
     // prevent access to existing instance properties except for id
     lockInstanceProperties(newInstance);
@@ -157,7 +160,7 @@ export function newInstance(name, initialProps = {}, mountTo = undefined) {
   };
 
   // add instance to the registry
-  registry.get(definition).add(newInstance);
+  definition.instances.add(newInstance);
 
   return newInstance;
 }
@@ -167,7 +170,7 @@ export function newInstance(name, initialProps = {}, mountTo = undefined) {
  */
 export function getInstance(name, id) {
   const definition = getDefinition(name);
-  return definition && Array.from(registry.get(definition)).find(instance => instance.id === id);
+  return definition && Array.from(definition.instances).find(instance => instance.id === id);
 }
 
 /**
@@ -181,20 +184,20 @@ export function isDefined(name) {
  * Test helper: get names of all components.
  */
 export function getComponentNames() {
-  return Array.from(registry.keys()).map(definition => definition.name);
+  return Object.keys(registry);
 }
 
 /**
  * Test helper: get all instances of the given component.
  */
 export function getComponentInstances(name) {
   const definition = getDefinition(name);
-  return definition ? Array.from(registry.get(definition).values()) : [];
+  return definition ? Array.from(definition.instances) : [];
 }
 
 /**
  * Test helper: remove all component data.
  */
 export function clearRegistry() {
-  registry.clear();
+  Object.keys(registry).forEach((k) => (delete registry[k]));
 }

app/javascript/spec/miq-component/helpers.spec.js

@@ -24,7 +24,7 @@ describe('Helpers', () => {
       },
     ];
 
-    define('FooComponent', {}, testInstances);
+    define('FooComponent', {}, { instances: testInstances });
 
     cleanVirtualDom();
     expect(destroy1).not.toHaveBeenCalled();

app/javascript/spec/miq-component/miq-component.spec.js

@@ -43,14 +43,24 @@ describe('Component API', () => {
     expect(getComponentNames()).toEqual(['FooComponent', 'BarComponent']);
   });
 
-  it('define method does nothing if the component name is already taken', () => {
+  it('define method throws if the component name is already taken', () => {
     define('FooComponent', {});
+    expect(() => {
+      define('FooComponent', {});
+    }).toThrow();
+    expect(getComponentNames()).toEqual(['FooComponent']);
+  });
+
+  it('define method passes twice with override option', () => {
     define('FooComponent', {});
+    define('FooComponent', {}, { override: true });
     expect(getComponentNames()).toEqual(['FooComponent']);
   });
 
   it('define method does nothing if the component name is not a string', () => {
-    define(123, {});
+    expect(() => {
+      define(123, {});
+    }).toThrow();
     expect(isDefined(123)).toBe(false);
     expect(getComponentNames()).toEqual([]);
   });
@@ -60,17 +70,17 @@ describe('Component API', () => {
       { id: 'first' }, { id: 'second' },
     ];
 
-    define('FooComponent', {}, testInstances);
+    define('FooComponent', {}, { instances: testInstances });
     expect(getInstance('FooComponent', 'first')).toBe(testInstances[0]);
     expect(getInstance('FooComponent', 'second')).toBe(testInstances[1]);
   });
 
   it('when passing existing instances, define method ensures a sane instance id', () => {
     const testInstances = [
-      { id: 'first' }, { id: 123 }, {},
+      { id: 'first' }, {}, {},
     ];
 
-    define('FooComponent', {}, testInstances);
+    define('FooComponent', {}, { instances: testInstances });
 
     const registeredInstances = getComponentInstances('FooComponent');
     expect(registeredInstances).toHaveLength(3);
@@ -84,7 +94,7 @@ describe('Component API', () => {
       { id: 'first' },
     ];
 
-    define('FooComponent', {}, testInstances);
+    define('FooComponent', {}, { instances: testInstances });
     expect(() => {
       testInstances[0].id = 'second';
     }).toThrow();
@@ -95,7 +105,7 @@ describe('Component API', () => {
       false, '', null, undefined, {},
     ];
 
-    define('FooComponent', {}, testInstances);
+    define('FooComponent', {}, { instances: testInstances });
 
     const registeredInstances = getComponentInstances('FooComponent');
     expect(registeredInstances).toHaveLength(1);
@@ -106,7 +116,7 @@ describe('Component API', () => {
     const testInstance = { id: 'first' };
 
     expect(() => {
-      define('FooComponent', {}, [testInstance, testInstance]);
+      define('FooComponent', {}, { instances: [testInstance, testInstance] });
     }).toThrow();
   });
 
@@ -116,7 +126,7 @@ describe('Component API', () => {
     ];
 
     expect(() => {
-      define('FooComponent', {}, testInstances);
+      define('FooComponent', {}, { instances: testInstances });
     }).toThrow();
   });
 
@@ -193,7 +203,7 @@ describe('Component API', () => {
     define('FooComponent', {
       create: jest.fn().mockName('testBlueprint.create')
         .mockImplementationOnce(() => ({ id: 'first', elementId: mountId }))
-        .mockImplementationOnce(() => ({ id: 123, elementId: mountId }))
+        .mockImplementationOnce(() => ({ elementId: mountId }))
         .mockImplementationOnce(() => ({ elementId: mountId })),
     });
 
@@ -424,7 +434,7 @@ describe('Component API', () => {
 
   it('sanitizeAndFreezeInstanceId ensures the instance id is sane and cannot be changed', () => {
     const testInstances = [
-      { id: 'first' }, { id: 123 }, {},
+      { id: 'first' }, {}, {},
     ];
 
     define('FooComponent', {});