ManageIQ / manageiq-ui-classic

Classic UI of ManageIQ
Apache License 2.0
50 stars 358 forks source link

Remove the need to deliver NodeJS in production #8300

Closed Fryguy closed 2 years ago

Fryguy commented 2 years ago

NodeJs present on the appliances / pods causes security scanning tools to flag us continually. If we can avoid the need to ship it (since we don't really use it), this might go a long way.

Work items:

jrafanie commented 2 years ago

NodeJs is only needed at runtime because we specify extra options to uglifier. Normally, you specify config.assets.js_compressor = :uglifier in you environments file and it's not eagerly loaded and therefore won't require execjs which requires nodejs. We need to provide a different configuration to uglifier, which means we cannot delay load uglifier. The question is whether these options for unused, keep_fargs, and keep_fnames are still necessary:

          config.assets.js_compressor = Uglifier.new(
            :compress => {
              :unused      => false,
              :keep_fargs  => true,
              :keep_fnames => true
            }

This requires uglifier and that will require nodejs via execjs.

Our options are to see if we can use the default options for uglifier and discard this:

{
  :unused      => false,
  :keep_fargs  => true,
  :keep_fnames => true
}

Or, find a way to provide customization for the delay loaded uglifier.

Fryguy commented 2 years ago

Getting this fixed will put less pressure on #8146

jrafanie commented 2 years ago
          config.assets.js_compressor = Uglifier.new(
            :compress => {
              :unused      => false,
              :keep_fargs  => true,
              :keep_fnames => true
            }

Note, this was added in https://github.com/ManageIQ/manageiq/pull/9878

"Uglifier removes unused functions that we call from controllers using RJS. This PR turns off this feature and also keeps unused arguments in functions."

Fryguy commented 2 years ago

I don't know why we'd want to keep that. @jeffibm do you have any thoughts / opinions?

jrafanie commented 2 years ago

I need to regenerate the assets clean BEFORE and AFTER removing that custom configuration for uglifier so I can see what exactly changes. It must check for changes as I saw no changes after running bin/update on master in production mode with the current code and then running it again with the config.assets.js_compressor = :uglifier (using the default configuration).

But yes, I really would prefer to just remove it and fix any fallout.

jrafanie commented 2 years ago

I'm comparing the assets compiled to public/assets and public/packs BEFORE and AFTER changing the uglifier options. To do this:

Generate the assets before making any changes

RAILS_ENV=production CONTAINER=true be rake assets:clobber
RAILS_ENV=production CONTAINER=true be rake assets:clean
RAILS_ENV=production CONTAINER=true bin/update
mv public/packs public/packs_before
mv public/assets public/assets_before

Changed engine.rb to delay load uglifier and accept the default options:

% git diff
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: lib/manageiq/ui/classic/engine.rb
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ lib/manageiq/ui/classic/engine.rb:41 @ module ManageIQ
         config.assets.paths << root.join('vendor', 'assets', 'stylesheets').to_s

         if Rails.env.production? || Rails.env.test?
-          require 'uglifier'
-          config.assets.js_compressor = Uglifier.new(
-            :compress => {
-              :unused      => false,
-              :keep_fargs  => true,
-              :keep_fnames => true
-            }
-          )
+          config.assets.js_compressor = :uglifier
         end

         def self.vmdb_plugin?
RAILS_ENV=production CONTAINER=true be rake assets:clobber
RAILS_ENV=production CONTAINER=true be rake assets:clean
RAILS_ENV=production CONTAINER=true bin/update
mv public/packs public/packs_after
mv public/assets public/assets_after

Diff the public/packs*

% diff -rq public/packs_*
# No changes

Diff public/assets*

% diff -rq public/assets_*
Files public/assets_after/.sprockets-manifest.json and public/assets_before/.sprockets-manifest.json differ
Only in public/assets_before/express/lib: application-e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49.js
Only in public/assets_before/express/lib: application-e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49.js.gz
Only in public/assets_after/express/lib: application-f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f.js
Only in public/assets_after/express/lib: application-f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f.js.gz

compare manifest.json

% json-diff public/assets_before/.sprockets-manifest.json public/assets_after/.sprockets-manifest.json
 {
   files: {
-    express/lib/application-e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49.js: {
-      logical_path: "express/lib/application.js"
-      mtime: "2022-06-15T12:30:15-04:00"
-      size: 5630
-      digest: "e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49"
-      integrity: "sha256-4NpwLV4JBOVOXo4/QbQq6SwFnkPkLo7HyR3OkmXH6kk="
-    }
+    express/lib/application-f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f.js: {
+      logical_path: "express/lib/application.js"
+      mtime: "2022-06-15T12:30:15-04:00"
+      size: 5459
+      digest: "f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f"
+      integrity: "sha256-+YO8rji2GQkdqH5U2v+DiK1fYASvbETBcb8GHxslWx8="
+    }
   }
   assets: {
-    express/lib/application.js: "express/lib/application-e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49.js"
+    express/lib/application.js: "express/lib/application-f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f.js"
   }
 }

Ok, so the manifest changes are fine.

Compare the minified application.js

% diff -y public/assets_before/express/lib/application-e0da702d5e0904e54e5e8e3f41b42ae92c059e43e42e8ec7c91dce9265c7ea49.js public/assets_after/express/lib/application-f983bcae38b619091da87e54daff8388ad5f6004af6c44c171bf061f1b255b1f.js
/*!                   / "use strict";function logerror(e){"test"!==this.get("env")&&c
 * express                  <
 * Copyright(c) 2009-2013 TJ Holowaychuk          <
 * Copyright(c) 2013 Roman Shtylman           <
 * Copyright(c) 2014-2015 Douglas Christopher Wilson        <
 * MIT Licensed                 <
 */                   <
"use strict";function logerror(e){"test"!==this.get("env")&&c <%

Note, the application.js has a difference near the end that is not shown there... I had to use pretty diff to see it. I don't know if it's changes we have to be concerned with.

image

Fryguy commented 2 years ago

@jrafanie If you look closely I don't think it's a real diff - notice on the left i = this.cache and on the right s = this.cache. So to compare you should rename everything on the right that is s to i. Annoyingly i is used for something else, so it's a pain. Perhaps pick new letters just for comparison, like x, y, z. In the end I don't think anything is actually different there.

Fryguy commented 2 years ago

Merged all PRs, so marking this as closed.

chessbyte commented 2 years ago

Less is more!™️