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

FIX: Use Terser for minification even if uglify-js is not available #13683

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Jul 9, 2021

In #12656, the default js minifier was switched to Terser from uglify-js, but in order for Terser to be used, uglify-js still has to be around. This fixes that.

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2021

CLA assistant check
All committers have signed the CLA.

@pmusaraj
Copy link
Contributor

pmusaraj commented Jul 9, 2021

Thanks @talyz, there is a TODO on line 105 in this file

# TODO: Remove uglifyjs when base image only includes terser

Since the base image now has terser, we can simply remove support for uglifyjs entirely. If you want to make that change in this PR, I'm happy to approve.

@talyz
Copy link
Contributor Author

talyz commented Jul 10, 2021

Sure! Maybe I should say that I'm not actually using the docker image - I maintain the discourse package for NixOS and its accompanying module (deployment scripts, essentially). In that package, we build the assets in a separate sandboxed build, so they're part of the package and not built on deployment. That's how I found out about this bug to begin with. Removing uglifyjs would add some extra work when backporting future releases to our stable branch, since we don't currently have terser in stable, but it's definitely solvable.

@talyz
Copy link
Contributor Author

talyz commented Jul 10, 2021

Added a fixup now. If it looks good to you and you prefer to remove uglifyjs, I'll squash it. :)

@pmusaraj
Copy link
Contributor

This looks good @talyz, I'll just wait for the tests to pass (they should) and then I'll approve. I'll handle the squashing when merging as well.

(Sorry to add work to your plate by removing uglifyjs, but it's something we were eventually going to do, anyway.)

@talyz
Copy link
Contributor Author

talyz commented Jul 10, 2021

Great! Yeah, it's no big deal, really. :)

@tgxworld
Copy link
Contributor

Changes look good to me so I'm merging this. Thank you for your contribution @talyz

@tgxworld tgxworld merged commit 7e52ead into discourse:master Jul 14, 2021
talyz added a commit to talyz/nixpkgs that referenced this pull request Jul 14, 2021
Discourse prefers to use `terser` when building js assets, see
discourse/discourse#12656. It still wants to
find `uglify-js` in order to not fall back to a ruby js compression
library, so let's keep it around for now. A fix for this has been
submitted upstream in
discourse/discourse#13683.
@talyz talyz deleted the terser-fix branch July 15, 2021 13:37
dpausp pushed a commit to dpausp/nixpkgs that referenced this pull request Oct 5, 2021
Discourse prefers to use `terser` when building js assets, see
discourse/discourse#12656. It still wants to
find `uglify-js` in order to not fall back to a ruby js compression
library, so let's keep it around for now. A fix for this has been
submitted upstream in
discourse/discourse#13683.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants