Fixes #20469 - Register react components from plugins #4733

xprazak2 · 2017-08-09T14:01:23Z

Plugins can register their own react components so that MountingService is aware of them.

//foreman_myplugin/webpack/index.js

import componentRegistry from 'foremanReact/components/componentRegistry';
import MyComponent from './components/MyComponent';
import MyOtherComponent from './components/MyOtherComponent';

componentRegistry.register({ name: 'MyComponent', type: MyComponent });
// store and data attributes are true by default
componentRegistry.register({ name: 'MyOtherComponent', type: MyOtherComponent, store: false, data: false });

// or to register multiple components:
componentRegistry.registerMultiple([
  { name: 'MyComponent', type: MyComponent },
  { name: 'MyOtherComponent', type: MyOtherComponent, store: false, data: false }
]);

components from plugins can be imported using an alias, so plugins can use components from other plugins:

// somewhere in foreman_somename/webpack/
import OtherComponent from 'myplugin/OtherComponent';

If you are not sure about alias for plugin, run script/plugin_webpack_directories.rb

We do not load all js on each page, plugins must make sure their js files are present.
The arguments of helper are ids that plugins use when registering:

# my_view.erb
<%= webpacked_plugins_js_for :foreman_ansible, :foreman_openscap %>

TODO:

Stylesheets in components are ignored for unknown reason, so Statistics page looks strange
register multiple components with one call

theforeman-bot · 2017-08-09T14:01:25Z

@xprazak2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #20429: Docker Compute Resource - URL should be required is the correct one
move ticket #20429 from Docker to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-09T14:01:26Z

Issues: #20429

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+        throw `Component not found:  ${name} among ${registeredComponents()}`
+      }
+      let ComponentName = currentComponent.type;
+      return <ComponentName data={ currentComponent.data ? data : undefined } store={ currentComponent.store ? store : undefined } />;


Line 66 exceeds the maximum line length of 100 max-len

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+      if (!currentComponent) {
+        throw `Component not found:  ${name} among ${registeredComponents()}`
+      }
+      let ComponentName = currentComponent.type;


Expected blank line after variable declarations newline-after-var

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+    let markup = (name, data, store) => {
+      let currentComponent = getComponent(name);
+      if (!currentComponent) {
+        throw `Component not found:  ${name} among ${registeredComponents()}`


Expected an object to be thrown no-throw-literal
Missing semicolon semi

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+    };
+
+    let markup = (name, data, store) => {
+      let currentComponent = getComponent(name);


Expected blank line after variable declarations newline-after-var

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+      }
+
+      registry[name] = component;
+      console.log('Component registered with name ' + name);


Unexpected console statement no-console

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+
+    let register = (name, component) => {
+      if (registry[name]) {
+        throw `Component name already taken: ${name}`;


Expected an object to be thrown no-throw-literal

houndci-bot · 2017-08-09T14:01:54Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+import PowerStatus from '../components/hosts/powerStatus/';
+import NotificationContainer from '../components/notifications/';
+import ToastsList from '../components/toastNotifications/';
+import StorageContainer from '../components/hosts/storage/vmware/';


'StorageContainer' is defined but never used no-unused-vars

houndci-bot · 2017-08-09T14:01:55Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

@@ -0,0 +1,72 @@
+import React from 'react';
+import store from '../redux';


'store' is defined but never used no-unused-vars

theforeman-bot · 2017-08-09T14:01:57Z

@xprazak2, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #20429: Docker Compute Resource - URL should be required is the correct one
move ticket #20429 from Docker to the Foreman project
or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ohadlevy · 2017-08-09T14:23:25Z

Looking good, I hope to try it soon.
For the js code style I suggest running lint or using something like prettier (with eslint)

houndci-bot · 2017-08-14T13:37:24Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

@@ -0,0 +1,85 @@
+import React from 'react';
+import store from '../redux';


'store' is defined but never used no-unused-vars

xprazak2 · 2017-08-14T13:55:20Z

config/webpack.config.js

+      foremanReactService:
+        path.join(__dirname,
+          '../webpack/assets/javascripts/services'),
+      foremanNodeModules:


This is here because of packaging-related changes. Plugins need to find foreman's node_modules somehow.

import React from 'foremanNodeModules/react';

does it mean that all plugins need to prefix their requirements to foreman node module? that would explain why in my tests i had a very large vendor file and a very large plugin specific file...I could see this being fairly annoying + hard to use npm packages that include common other libs (e.g. a react-date picker or something like that)

I agree it is a nuisance, but import React from 'react'; in plugins does not work and I haven't found a better solution yet. I am not sure what you mean about packages that include common other libs, could you explain please?

for example, if I import a 3rd party react component (lets say a date picker) then it code will include

import React from 'react';

This will:
a. make us pull another copy of react into the plugin node modules
b. will include another copy of react in the plugin bundle.

/cc @dLobatog - this is probably a side effect we did not expect.

ohadlevy

looks like a great start, I've left a few comments inline.

Other things to consider for this pr:

tests
ability for a plugin to add customer actions/reducers.
reuse the same eslint configuration as core by default.

does a factory (such as described at https://medium.com/@SntsDev/the-factory-pattern-in-js-es6-78f0afad17e9) make any sense in this context?

thanks @xprazak2 !

ohadlevy · 2017-08-14T19:51:00Z

app/views/layouts/base.html.erb

@@ -17,6 +17,9 @@
    <%= csrf_meta_tags %>
    <%= javascript_include_tag *webpack_asset_paths('vendor', :extension => 'js'), "data-turbolinks-track" => true %>
    <%= javascript_include_tag *webpack_asset_paths('bundle', :extension => 'js'), "data-turbolinks-track" => true %>
+    <% webpacked_plugins_js_tags.each do |item| %>
+      <%= javascript_include_tag *item, "data-turbolinks-track" => true %>


should this be optional? e.g. maybe not load all js on all pages?

ohadlevy · 2017-08-14T19:52:43Z

config/webpack.config.js

+      foremanReactService:
+        path.join(__dirname,
+          '../webpack/assets/javascripts/services'),
+      foremanNodeModules:


does it mean that all plugins need to prefix their requirements to foreman node module? that would explain why in my tests i had a very large vendor file and a very large plugin specific file...I could see this being fairly annoying + hard to use npm packages that include common other libs (e.g. a react-date picker or something like that)

ohadlevy · 2017-08-14T19:53:57Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+    let registry = {
+      PieChart: {
+        type: PieChart,
+        store: true,


can store and data default to true (so you dry)?

ohadlevy · 2017-08-14T19:54:44Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+      }
+    };
+
+    let register = (name, component) => {


why let vs const?

ohadlevy · 2017-08-14T19:56:08Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+      let currentComponent = getComponent(name);
+
+      if (!currentComponent) {
+        throw new Error(`Component not found:  ${name} among ${registeredComponents()}`);


nitpick: extra whitespace between found: and ${name}

houndci-bot · 2017-08-18T14:38:17Z

webpack/assets/javascripts/react_app/common/componentRegistry.js

+    const currentComponent = this.getComponent(name);
+
+    if (!currentComponent) {
+      throw new Error(`Component not found:  ${name} among ${registeredComponents()}`);


'registeredComponents' is not defined no-undef

houndci-bot · 2017-08-18T14:38:17Z

script/plugin_webpack_directories.js

+  return splitDirs.length > 2 ? splitDirs[1] : pluginDirs;
+};
+
+var webpackDirs = execSync(path.join(__dirname, './plugin_webpack_directories.rb'), {


Unexpected var, use let or const instead no-var

houndci-bot · 2017-08-18T14:38:17Z

script/plugin_webpack_directories.js

+// If we get multiple lines, then the plugin_webpack_directories.rb script
+// has on the stdout more that just the JSON we want, so we use newline to split and check.
+var sanitizeWebpackDirs = function (pluginDirs) {
+  var splitDirs = pluginDirs.toString().split("\n").reverse();


Unexpected var, use let or const instead no-var
Strings must use singlequote quotes

houndci-bot · 2017-08-18T14:38:17Z

script/plugin_webpack_directories.js

+
+// If we get multiple lines, then the plugin_webpack_directories.rb script
+// has on the stdout more that just the JSON we want, so we use newline to split and check.
+var sanitizeWebpackDirs = function (pluginDirs) {


Unexpected var, use let or const instead no-var

houndci-bot · 2017-08-18T14:38:17Z

script/plugin_webpack_directories.js

+'use strict';
+
+var execSync = require('child_process').execSync;
+var path = require('path');


Unexpected var, use let or const instead no-var

houndci-bot · 2017-08-18T14:38:17Z

script/plugin_webpack_directories.js

@@ -0,0 +1,18 @@
+'use strict';
+
+var execSync = require('child_process').execSync;


Unexpected var, use let or const instead no-var

houndci-bot · 2017-08-18T14:38:17Z

app/helpers/reactjs_helper.rb

+
+    bundle_names.map do |name|
+      javascript_include_tag *webpack_asset_paths(name, :extension => 'js'), "data-turbolinks-track" => true
+    end.reduce(&:concat).html_safe


Prefer inject over reduce.

houndci-bot · 2017-08-18T14:38:17Z

app/helpers/reactjs_helper.rb

+    bundle_names = requested_plugins.map { |plugin| Foreman::Plugin.bundle_name plugin }
+
+    bundle_names.map do |name|
+      javascript_include_tag *webpack_asset_paths(name, :extension => 'js'), "data-turbolinks-track" => true


Ambiguous splat operator. Parenthesize the method arguments if it's surely a splat operator, or add a whitespace to the right of the * if it should be a multiplication.

xprazak2 · 2017-08-18T15:03:59Z

Changes:

I turned the registry into a factory as described in the blog post, data and store attributes are true by default, which made code a bit DRYer.

import MyComponent from './MyComponent';
componentRegistry.register({ name: 'MyComponent', type: MyComponent });

registerMultiple accepts an array of objects with the same format.

npm run lint lints all the webpacked plugins now as well.
Including everything into all pages seems a bit too much. It will be therefore responsibility of a plugin to request the js files to be included:

# my_view.erb
<%= webpacked_plugins_js_for :foreman_ansible, :foreman_openscap %>

The arguments of helper are ids that plugins use when registering.

still missing tests, adding actions and better path resolution

ohadlevy · 2017-08-21T10:35:41Z

@xprazak2 interesting to follow ManageIQ/manageiq-ui-classic#1944 as well, as they implement plugability in the redux concept, i think this two (this pr and manageiq) will complete each other.

timogoebel · 2017-08-31T13:13:41Z

We gave this a shot. Works nicely.
@ohadlevy: Anything missing?

ohadlevy · 2017-08-31T13:18:42Z

package.json

@@ -77,7 +77,7 @@
    "webpack-stats-plugin": "^0.1.5"
  },
  "scripts": {
-    "lint": "./node_modules/.bin/eslint -c .eslintrc webpack/ || exit 0",
+    "lint": "./node_modules/.bin/eslint -c .eslintrc webpack/ $(./script/foreman_plugins_eslint.js) || exit 0",


nice :) we should do the same for rubocop

xprazak2 · 2017-10-03T07:13:00Z

I fixed the code climate error, is there anything else I should fix? Do you want me to incorporate the change that prevents node modules to appear twice?

xprazak2 · 2017-10-03T08:22:53Z

[test katello]

ohadlevy · 2017-10-03T08:48:57Z

webpack/assets/javascripts/react_app/components/componentRegistry.js

+  },
+
+  registerMultiple(componentObjs) {
+    return _.forEach(componentObjs, (obj) => {


maybe just import foreach instead of the entire lodash, e.g.:

import foreach from 'lodash/foreach' return foreach(componentObjs, (obj) => {)

xprazak2 · 2017-10-03T10:29:56Z

I now import just the functions instead of the whole lodash

ohadlevy · 2017-10-03T11:54:07Z

app/helpers/reactjs_helper.rb

@@ -4,4 +4,31 @@ def mount_react_component(name, selector, data = [])
      "$(tfm.reactMounter.mount('#{name}', '#{selector}', #{data}));".html_safe
    end
  end
+
+  def webpacked_plugins_js_for(*plugin_names)


@xprazak2 would you mind adding tests for these helpers?

xprazak2 · 2017-10-03T13:14:24Z

I added a couple of tests to cover new helpers.

ohadlevy · 2017-10-03T13:51:13Z

test/helpers/reactjs_helper_test.rb

+  Foreman::Plugin.register(:foreman_angular) {}
+
+  def webpack_asset_paths(bundle_name, opts)
+    ["<script src=\"https://foreman.example.com:3808/webpack/#{bundle_name}.js\"></script>"]


shouldnt this be on the same url as the server in prod/test?

Does it matter? It is just a stub for test, the method comes from webpack-rails and I cannot use it directly in test because it complains about missing manifest.

ohadlevy · 2017-10-03T14:10:02Z

oh, ,right i didn't read it correctly - thanks.

mmoll · 2017-10-03T15:03:40Z

[test katello]

waldenraines

What is here worked well for us after @ohadlevy's change in #4733 (comment) but I don't see that included in the webpack config yet. Will approve after that is added.

waldenraines · 2017-10-03T15:30:56Z

config/webpack.config.js

@@ -52,11 +50,16 @@ var config = {
      path.join(__dirname, '..', 'webpack'),


Do you mind making the change @ohadlevy mentions in #4733 (comment)?

in that case 👍 from my side to add those changes. /cc @dLobatog as it seems we would need to find a different approach to package this.

It did indeed broke packaging which means our nightlies are now broken. Given the current load on the packaging and release team it might be wise to slow down these changes, at least until 1.16 is out.

xprazak2 · 2017-10-04T06:01:18Z

I replaced 'node_modules/' with path.join(__dirname, '..', 'node_modules'). I also removed the alias for node_modules as it is no longer needed, now we can use

import React from 'react'

in plugins as well.

ohadlevy · 2017-10-04T08:14:22Z

Thanks @xprazak2!

dLobatog · 2017-10-06T09:54:20Z

This PR removed the fix in http://projects.theforeman.org/issues/20511, which in turn makes Foreman nightlies unable to build again http://koji.katello.org/kojifiles/work/tasks/5312/35312/build.log 'ModuleNotFoundError: Module not found: Error: Can't resolve 'object-assign' in '/usr/lib/node_modules/react/lib''. I'll fix that on another PR.

ohadlevy · 2017-10-06T10:57:42Z

@dLobatog as mentioned earlier in this thread, the packaging solution conflict with our plugin assets requirement (as it duplicate the libraries in the bundle across plugins).

theforeman-bot added the Waiting on contributor label Aug 9, 2017

theforeman-bot added Needs testing Not yet reviewed labels Aug 9, 2017

houndci-bot reviewed Aug 9, 2017

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 9, 2017

xprazak2 force-pushed the register-components branch from 550073d to a372615 Compare August 9, 2017 14:02

theforeman-bot added Needs testing and removed Waiting on contributor labels Aug 9, 2017

xprazak2 force-pushed the register-components branch from a372615 to b3988c4 Compare August 14, 2017 13:37

houndci-bot reviewed Aug 14, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch from b3988c4 to 493a67c Compare August 14, 2017 13:50

xprazak2 commented Aug 14, 2017

View reviewed changes

xprazak2 changed the title ~~Fixes #20429 - [wip] Register react components from plugins~~ Fixes #20429 - Register react components from plugins Aug 14, 2017

ohadlevy suggested changes Aug 14, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Aug 14, 2017

xprazak2 force-pushed the register-components branch from 493a67c to 3cb8db2 Compare August 18, 2017 14:38

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Aug 18, 2017

houndci-bot reviewed Aug 18, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch from 3cb8db2 to bc326dd Compare August 18, 2017 14:44

ohadlevy reviewed Aug 31, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch 2 times, most recently from c83509c to 9e25134 Compare October 3, 2017 07:07

ohadlevy reviewed Oct 3, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch from 9e25134 to 24b1175 Compare October 3, 2017 10:28

ohadlevy reviewed Oct 3, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch from 24b1175 to 911ba90 Compare October 3, 2017 13:11

ohadlevy reviewed Oct 3, 2017

View reviewed changes

xprazak2 force-pushed the register-components branch from 911ba90 to 6b49bc8 Compare October 3, 2017 14:26

ohadlevy approved these changes Oct 3, 2017

View reviewed changes

waldenraines suggested changes Oct 3, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 3, 2017

Fixes #20469 - Register react components from plugins

404597e

xprazak2 force-pushed the register-components branch from 6b49bc8 to 404597e Compare October 4, 2017 05:54

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 4, 2017

ohadlevy merged commit 8ec9fb0 into theforeman:develop Oct 4, 2017

This was referenced Nov 22, 2017

Fixes #21702 - Add reducer registration to redux #5014

Merged

Fixes #21203 - Move from webpack-rails to webpacker #5033

Closed

		@@ -0,0 +1,72 @@
		import React from 'react';
		import store from '../redux';

		@@ -0,0 +1,85 @@
		import React from 'react';
		import store from '../redux';

		@@ -0,0 +1,18 @@
		'use strict';

		var execSync = require('child_process').execSync;

		@@ -52,11 +50,16 @@ var config = {
		path.join(__dirname, '..', 'webpack'),

Fixes #20469 - Register react components from plugins #4733

Fixes #20469 - Register react components from plugins #4733

Conversation

xprazak2 commented Aug 9, 2017 • edited Loading

theforeman-bot commented Aug 9, 2017

theforeman-bot commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Aug 9, 2017

ohadlevy commented Aug 9, 2017

Choose a reason for hiding this comment

xprazak2 Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

ohadlevy Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

xprazak2 Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xprazak2 commented Aug 18, 2017

ohadlevy commented Aug 21, 2017

timogoebel commented Aug 31, 2017

ohadlevy Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

xprazak2 commented Oct 3, 2017

xprazak2 commented Oct 3, 2017

Choose a reason for hiding this comment

xprazak2 commented Oct 3, 2017

Choose a reason for hiding this comment

xprazak2 commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Oct 3, 2017 via email

mmoll commented Oct 3, 2017

waldenraines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xprazak2 commented Oct 4, 2017 • edited Loading

ohadlevy commented Oct 4, 2017

dLobatog commented Oct 6, 2017

ohadlevy commented Oct 6, 2017

xprazak2 commented Aug 9, 2017 •

edited

Loading

xprazak2 Aug 14, 2017 •

edited

Loading

ohadlevy Aug 14, 2017 •

edited

Loading

xprazak2 Aug 18, 2017 •

edited

Loading

ohadlevy Aug 14, 2017 •

edited

Loading

ohadlevy Aug 31, 2017 •

edited

Loading

xprazak2 commented Oct 4, 2017 •

edited

Loading