Merge ~weii-wang/charm-k8s-discourse:fix-terser-precompile into charm-k8s-discourse:master

Proposed by Weii Wang
Status: Merged
Approved by: Tom Haddon
Approved revision: ada4ba97b28ab53dd0a619b3a9325b7a981afac1
Merge reported by: Tom Haddon
Merged at revision: ada4ba97b28ab53dd0a619b3a9325b7a981afac1
Proposed branch: ~weii-wang/charm-k8s-discourse:fix-terser-precompile
Merge into: charm-k8s-discourse:master
Diff against target: 80 lines (+10/-26)
4 files modified
dev/null (+0/-13)
image/build_scripts/build_app (+0/-3)
image/build_scripts/get_app_dependencies (+9/-1)
image/scripts/pod_setup (+1/-9)
Reviewer Review Type Date Requested Status
Tom Haddon Approve
Barry Price Approve
Franco Luciano Forneron Buschiazzo Pending
Review via email: mp+427723@code.launchpad.net

Commit message

Enable terser to uglify JS assets

Set up the terser package and the SKIP_NODE_UGLIFY environment variable
correctly so discourse will use terser instead of uglifyjs to compress
all the Javascript assets.

LP: #1952681

To post a comment you must log in.
Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Barry Price (barryprice) wrote :

Thanks for your work on this, looks good to me - couple of minor nitpicks inline below.

review: Approve
Revision history for this message
Weii Wang (weii-wang) wrote :

> Thanks for your work on this, looks good to me - couple of minor nitpicks
> inline below.

I made some modifications based on that, new commit has been squashed and uploaded.
Thanks!

Revision history for this message
Tom Haddon (mthaddon) wrote :

LGTM, thx

review: Approve
Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

Change successfully merged at revision d86ae45b79d2685989b869200a3a2566f832ed3b

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/image/build_scripts/build_app b/image/build_scripts/build_app
2index 9d6d04a..6a0333c 100755
3--- a/image/build_scripts/build_app
4+++ b/image/build_scripts/build_app
5@@ -16,9 +16,6 @@ git clone --depth 1 --branch ${CONTAINER_APP_VERSION} https://github.com/discour
6 # Apply patch for LP#1903695
7 git -C "${CONTAINER_APP_VERSION}" apply /srv/build_scripts/lp1903695.patch
8
9-# Apply patch to use terser rather than uglifier
10-git -C "${CONTAINER_APP_VERSION}" apply /srv/build_scripts/terser.patch
11-
12 chown -R ${CONTAINER_APP_USERNAME}:${CONTAINER_APP_GROUP} ${CONTAINER_APP_VERSION}
13 ln -s ${CONTAINER_APP_ROOT}/${CONTAINER_APP_VERSION} app
14 cd ${CONTAINER_APP_ROOT}/app
15diff --git a/image/build_scripts/get_app_dependencies b/image/build_scripts/get_app_dependencies
16index 8f9a0f6..2295656 100755
17--- a/image/build_scripts/get_app_dependencies
18+++ b/image/build_scripts/get_app_dependencies
19@@ -16,7 +16,7 @@ apt-get install -y brotli \
20 libxml2-dev \
21 libxslt1-dev \
22 libz-dev \
23- node-terser \
24+ uglifyjs.terser \
25 node-uglify \
26 optipng \
27 pngquant \
28@@ -27,3 +27,11 @@ apt-get install -y brotli \
29 ruby2.7-dev \
30 zlib1g-dev
31
32+# Older versions of the uglifyjs.terser package install a uglifyjs.terser
33+# command but not the terser command, terser command exists in $PATH is
34+# vital to trigger js assets compression using node. Manually create the
35+# terser command if it does not exist. Please remove this line if the
36+# base image is >= 22.04. Also, please consider removing the node-uglify
37+# when upgrading to discourse >= 2.8.0, since the node-uglify is not
38+# required to trigger node js assets compression, only terser will do fine.
39+which terser || ln -s $(which uglifyjs.terser) /usr/local/bin/terser
40\ No newline at end of file
41diff --git a/image/build_scripts/terser.patch b/image/build_scripts/terser.patch
42deleted file mode 100644
43index 55e20ff..0000000
44--- a/image/build_scripts/terser.patch
45+++ /dev/null
46@@ -1,13 +0,0 @@
47-diff --git a/config/environments/production.rb b/config/environments/production.rb
48-index a523888a8d..7f41985b17 100644
49---- a/config/environments/production.rb
50-+++ b/config/environments/production.rb
51-@@ -14,7 +14,7 @@ Discourse::Application.configure do
52- # Disable Rails's static asset server (Apache or nginx will already do this)
53- config.public_file_server.enabled = GlobalSetting.serve_static_assets || false
54-
55-- config.assets.js_compressor = :uglifier
56-+ config.assets.js_compressor = :terser
57-
58- # stuff should be pre-compiled
59- config.assets.compile = false
60diff --git a/image/scripts/pod_setup b/image/scripts/pod_setup
61index 4893723..ca5f8d4 100755
62--- a/image/scripts/pod_setup
63+++ b/image/scripts/pod_setup
64@@ -36,15 +36,7 @@ else
65 fi
66 ### MIGRATION LOCK WORKAROUND ENDS HERE
67
68-# It is safe to build assets in the background to improve startup time.
69-export SKIP_NODE_UGLIFY="true"
70-
71-# LP#1952681: First run consistently fails to process discourse/tests/theme_qunit_helper JS
72-su -s /bin/bash -c "bin/bundle exec rake assets:precompile RAILS_ENV=$RAILS_ENV" ${CONTAINER_APP_USERNAME} 2>&1 |sed 's/^/asset-build-1: /'
73-# LP#1952681: Second run consistently fails to process application JS
74-su -s /bin/bash -c "bin/bundle exec rake assets:precompile RAILS_ENV=$RAILS_ENV" ${CONTAINER_APP_USERNAME} 2>&1 |sed 's/^/asset-build-2: /'
75-# LP#1952681: Third run consistently succeeds
76-su -s /bin/bash -c "bin/bundle exec rake assets:precompile RAILS_ENV=$RAILS_ENV" ${CONTAINER_APP_USERNAME} 2>&1 |sed 's/^/asset-build-3: /'
77+su -s /bin/bash -c "bin/bundle exec rake assets:precompile RAILS_ENV=$RAILS_ENV" ${CONTAINER_APP_USERNAME} 2>&1 | sed 's/^/asset-build: /'
78
79 if [ ! -z "${DISCOURSE_USE_S3}" ] && [ "${DISCOURSE_USE_S3}" == "true" ]; then
80 echo "Discourse is configured to use S3:"

Subscribers

People subscribed via source and target branches