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

Latest uglifier, 4.2.0 prevents runtime uglifier asset compilation #7835

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

jrafanie
Copy link
Member

ExecJS 2.8.1 requires uglifier 4.2.0 to defer uglifier asset compilation until asset
compilation time, see: rails/execjs#105

Trying to compile the uglfier assets in production mode in an environment with
dynatrace was causing conflicts with the execjs node runtime and the dynatrace
reporting mechanism. We should be able to avoid this entirely if we only call
out to node for asset compilation when we do explicit asset compilation at build
time in a controlled environment without dynatrace instead of at runtime, which
we cannot control.

ExecJS 2.8.1 requires uglifier 4.2.0 to defer uglifier asset compilation until asset
compilation time, see: rails/execjs#105

Trying to compile the uglfier assets in production mode in an environment with
dynatrace was causing conflicts with the execjs node runtime and the dynatrace
reporting mechanism.  We should be able to avoid this entirely if we only call
out to node for asset compilation when we do explicit asset compilation at build
time in a controlled environment without dynatrace instead of at runtime, which
we cannot control.
@jrafanie
Copy link
Member Author

@Fryguy I'm not sure how to test this since it's a build/test time only set of changes. We can do your idea of raising an error if it tries to hit this code in production: https://github.com/lautis/uglifier/blob/v4.2.0/lib/uglifier.rb#L183-L184, but I'm not sure how to verify the uglifier upgrade doesn't change behavior since the underlying UglifyJS changed with most version changes from 3.0.4 to 4.2.0: https://github.com/lautis/uglifier/blob/master/CHANGELOG.md#420-25-september-2019

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put into an rpm and installed on an appliance

I verified:

  1. the right version of uglifier was installed
  2. the rpms were built fine
  3. the ui came up and the dashboard and new provider form were in tact
  4. no error messages grep ugl logs/*.log
  5. javascript console showed no errors

@kbrock
Copy link
Member

kbrock commented Aug 18, 2021

hey @agrare and @Fryguy could you take a peek and make sure this meets our needs?

@agrare
Copy link
Member

agrare commented Aug 19, 2021

If the RPM build works that's good enough for me, not sure the container build side would be any different
@Fryguy ?

@Fryguy Fryguy merged commit 16df3d8 into ManageIQ:master Aug 20, 2021
@Fryguy Fryguy self-assigned this Aug 20, 2021
@jrafanie jrafanie deleted the update_to_latest_uglfier_execjs branch August 20, 2021 17:49
@bdunne
Copy link
Member

bdunne commented Aug 23, 2021

This requires changes to Gemfile.lock.release in the core repo.

@Fryguy
Copy link
Member

Fryguy commented Aug 23, 2021

@jrafanie A conflict occurred during the backport of this pull request to lasker.

If this pull request is based on another pull request that has not been marked for backport, add the appropriate labels to the other pull request. Otherwise, please create a new pull request direct to the lasker branch in order to resolve this.

Conflict details:

diff --cc manageiq-ui-classic.gemspec
index 757932268d,855bb98183..0000000000
--- a/manageiq-ui-classic.gemspec
+++ b/manageiq-ui-classic.gemspec
@@@ -21,8 -21,7 +21,12 @@@ Gem::Specification.new do |s
  
    s.add_dependency "rails", "~> 6.0.0"
  
++<<<<<<< HEAD
 +  s.add_dependency "coffee-rails"
 +  s.add_dependency "execjs", "2.7.0" # HACK: 2.8.1 is causing node to exit 1 when compiling uglify's sourcecode in some environments. See https://github.com/rails/execjs/issues #105
++=======
+   s.add_dependency "execjs", "2.8.1" # Note: 2.8.1 requires uglifier 4.2.0 to defer uglifier asset compilation until asset compilation time: https://github.com/rails/execjs/issues/105
++>>>>>>> 16df3d8cbe... Merge pull request #7835 from jrafanie/update_to_latest_uglfier_execjs
    s.add_dependency "font-fabulous", "~> 1.0.5"
    s.add_dependency "high_voltage", "~> 3.0.0"
    s.add_dependency "more_core_extensions", ">= 3.2", "< 5"

@Fryguy
Copy link
Member

Fryguy commented Aug 23, 2021

@jrafanie I'll manually backport as this is trivial.

Fryguy added a commit that referenced this pull request Aug 23, 2021
Latest uglifier, 4.2.0 prevents runtime uglifier asset compilation

(cherry picked from commit 16df3d8)
@Fryguy
Copy link
Member

Fryguy commented Aug 23, 2021

lasker backport details:

commit 4340ca772b68f7ea0b53b084fc365250028fe70f (HEAD -> lasker, upstream/lasker)
Author: Jason Frey <[email protected]>
Date:   Fri Aug 20 13:13:49 2021 -0400

    Merge pull request #7835 from jrafanie/update_to_latest_uglfier_execjs

    Latest uglifier, 4.2.0 prevents runtime uglifier asset compilation

    (cherry picked from commit 16df3d8cbef515204b3bc0fbb5b1d302762524ea)

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.

6 participants