[SE-3401] Updates JakePackage.zip to make JS minification consistent across different machines

[nizarmah]

When generating the minified files for the TinyMCE static files using the vendor_extra/tinymce/JakePackage.zip, there's often a problem that was happening where the files are different after running the following command:

jake minify bundle[themes:modern,plugins:advlist,anchor,autolink,charmap,code,codemirror,contextmenu,image,insertdatetime,link,lists,media,paste,print,save,searchreplace,table,textcolor,visualblocks]

This becomes an issue when installing a new TinyMCE plugin on different machines and re-running jake minify bundle, because the files are not consistent on different machines. This results in a different commons.js file hash on the different machines, which makes it not possible to use multiple instances with a load balancer.

Some of the issues are, amdlc compiles the minified output for a plugin, and then uglify-js doesn't update the file. Therefore, removed the minification process from the amdlc step and reverted minification to uglify-js.

There are other reasons why the commons.js file hash is different, which are mentioned in PR edx#24990.
Therefore, this is a follow-up PR to edx#24990.

This PR simply adds a specific configuration for uglify-js, which is:

mangle : {
	sort: true,
	toplevel: false
},
compress: {
	cascade: false,
	comparisons: false,
	dead_code: true,
	if_return: true
},
output: {
	ascii_only: true,
	beautify: false
},
toplevel: false

One of the most important is, sort which makes the mangled variable names consistent on different runs. Also, comparisons and if_return are specified because they were sometimes applied, while others times, they weren't.

JIRA tickets: SE-3401, SE-3101, SE-3381, OSPR-5047

Testing instructions:

1. Change your directory to common/static/js/vendor/tinymce/
2. Once you are there, run unzip ../../../../../vendor_extra/tinymce/JakePackage.zip
3. Clean the existing npm modules, rm -rf node_modules
4. Install the npm dependencies, npm install
5. Run the jake clean command, npx jake clean
6. Run the jake command, mentioned below.
7. Commit the changes since I didn't update the existing minified files.
8. Run the jake command, mentioned below, again.
9. Make sure no changes have been made to the files.
10. Verify all plugin.min.js were generated correctly using: grep -Er "^undefined$".

npx jake minify bundle[themes:modern,plugins:advlist,anchor,autolink,charmap,code,codemirror,contextmenu,image,insertdatetime,link,lists,media,paste,print,save,searchreplace,table,textcolor,visualblocks]

Author notes and concerns:

1. Do you think it is better to update the minified files and commit those changes? Please let me know.

Files changed:

package.json:

        "repository": {
-               "type" : "git",
-               "url" : "https://github.com/tinymce/tinymce.git"
+               "type": "git",
+               "url": "https://github.com/tinymce/tinymce.git"
        },
        "description": "TinyMCE rich text editor",
        "author": "Johan Sörlin <spocke@moxiecode.com>",
-       "bugs": { "url" : "http://www.tinymce.com/develop/bugtracker.php" },
+       "bugs": {
+               "url": "http://www.tinymce.com/develop/bugtracker.php"
+       },
        "private": true,
        "engines": {
-               "node" : ">=0.10.26"
+               "node": ">=0.10.26"
        },
        "devDependencies": {
-               "jake": ">= 0.7.0",
-               "amdlc": ">= 0.0.2",
-               "jshint": ">= 2.1.4",
-               "eslint": ">= 0.4.2",
-               "uglify-js": ">= 2.0.0",
-               "glob": ">= 3.1.12",
-               "moxie-zip": ">= 0.0.1",
-               "less": ">= 1.3.1",
-               "coverjs": ">= 0.0.14"
+               "jake": "0.7.10",
+               "amdlc": "0.1.2",
+               "jshint": "2.4.4",
+               "eslint": "0.4.5",
+               "uglify-js": "2.4.13",
+               "glob": "3.2.9",
+               "moxie-zip": "0.0.3",
+               "less": "1.7.0",
+               "coverjs": "0.0.14"
        },

package-lock.json

+ everything...

Jakefile.js

@@ -189,7 +189,6 @@ task("minify-pasteplugin", [], function() {
                baseDir: "js/tinymce/plugins/paste/classes",
                rootNS: "tinymce.pasteplugin",
                outputSource: "js/tinymce/plugins/paste/plugin.js",
-               outputMinified: "js/tinymce/plugins/paste/plugin.min.js",
                outputDev: "js/tinymce/plugins/paste/plugin.dev.js",
                verbose: false,
                expose: "public",
@@ -203,7 +202,6 @@ task("minify-spellcheckerplugin", [], function() {
         baseDir: "js/tinymce/plugins/spellchecker/classes",
         rootNS: "tinymce.spellcheckerplugin",
         outputSource: "js/tinymce/plugins/spellchecker/plugin.js",
-        outputMinified: "js/tinymce/plugins/spellchecker/plugin.min.js",
         outputDev: "js/tinymce/plugins/spellchecker/plugin.dev.js",
         verbose: false,
         expose: "public",
@@ -217,7 +215,6 @@ task("minify-tableplugin", [], function() {
         baseDir: "js/tinymce/plugins/table/classes",
         rootNS: "tinymce.tableplugin",
         outputSource: "js/tinymce/plugins/table/plugin.js",
-        outputMinified: "js/tinymce/plugins/table/plugin.min.js",
         outputDev: "js/tinymce/plugins/table/plugin.dev.js",
         verbose: false,
         expose: "public",
@@ -663,3 +660,20 @@ task("clean", [], function () {
        });
 });
 
+desc("Cleans the minified JavaScript for plugins and themes");
+task("clean-js", [], function () {
+       [
+               "tmp/*",
+               "js/tinymce/tinymce*",
+               "js/tinymce/*.min.js",
+               "js/tinymce/*.dev.js",
+               "js/tinymce/plugins/table/plugin.js",
+               "js/tinymce/plugins/!(image|media)/plugin.min.js",
+               "js/tinymce/themes/*/theme.min.js",
+       ].forEach(function(pattern) {
+               glob.sync(pattern).forEach(function(filePath) {
+                       fs.unlinkSync(filePath);
+               });
+       });
+});
+

tools/BuildTools.js

@@ -31,12 +31,23 @@ exports.uglify = function(options) {
        var UglifyJS = require("uglify-js");
        var filePaths = [];
 
-       options = extend({
-               mangle : true,
-               toplevel : false,
-               no_functions : false,
-               ascii_only: true
-       }, options);
+       var uglifyOptions = {
+               mangle : {
+                       sort: true,
+                       toplevel: false
+               },
+               compress: {
+                       cascade: false,
+                       comparisons: false,
+                       dead_code: true,
+                       if_return: true
+               },
+               output: {
+                       ascii_only: true,
+                       beautify: false
+               },
+               toplevel: false
+       };
 
        var toFileModTime = getFileModTime(options.to);
        var fromFileModTime = 0;
@@ -58,8 +69,7 @@ exports.uglify = function(options) {
        }
 
        if (options.force === true || fromFileModTime !== toFileModTime) {
-               var result = UglifyJS.minify(filePaths, {
-               });
+               var result = UglifyJS.minify(filePaths, uglifyOptions);
 
                fs.writeFileSync(options.to, result.code);
                setFileModTime(options.to, fromFileModTime);

Reviewers

• @pkulkark
• edX reviewer[s] TBD

[openedx-webhooks]

Thanks for the pull request, @nizarmah! I've created OSPR-5047 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[natabene]

@nizarmah Thank you for your contribution. Please let me know once it ready for our review.

[pkulkark]

@nizarmah Since this only adds the zipped folder, could you include the exact changes you made in a diff format in the description? Also I think it's a good idea to include the minified files.

[nizarmah]

@pkulkark yes, definitely, I'll ping you once that's ready 👍

[nizarmah]

Alright, @pkulkark, added all the changes as they were using git diff. This should be ready for your review 👍

[pkulkark]

Thanks @nizarmah. LGTM 👍

• I tested this: verified that JS minification is consistent across different machines.
• I read through the code

[nizarmah]

@natabene this is ready for edX's review 👍 😃

[natabene]

@nizarmah Thank you for your contribution. I am lining this up for our review.

[nizarmah]

@natabene are there any updates on this?

This is currently blocking the merge of https://github.com/edx/configuration/pull/6179

---

Also @doctoryes, based on the discussion from a different pull request, what are your thoughts about removing the zip file, and moving the Jakefile.js and its contents to common/static/js/vendor/tinymce/, that way, everything needed already exists there, except the npm_modules?

We will soon have an ansible role for re-building the TinyMCE files, so it might be possible to simplify the process and get remove the ZIP file.

[natabene]

@nizarmah No updates yet, we didn't have enough bandwidth to address this. Hopefully soon.

[doctoryes]

@nizarmah The commit was six years ago so I've lost context. But I recall needing to make the change mentioned in the commit message here: edx@2a8529d
My goal was to fix the bug while keeping the Studio editor functional. I have no opinion on this change. Feel free to make whatever changes you need, while continuing to keep the editor functional.

[nizarmah]

Thanks for the reply on this @doctoryes 🙂

@pomegranited, since you reviewed https://github.com/edx/configuration/pull/6179, what are your suggestions here? Should I submit follow-up requests to both the configuration and the platform or make changes to the existing two pull requests?

The reason I'm asking is that the new follow-up requests will depend on each other, similarly to how the current pull requests do. So, it would create additional blocks.

[pomegranited]

@nizarmah I think KISS applies here -- just leave this PR as-is and don't worry about a follow-up. If we haven't touched this package in 4 years, then I don't think there's a reason to be in an all-fired hurry to fix what ain't broke. 😄

[bradenmacdonald]

@natabene Is it OK if I review this one, since it relates to another PR I'm reviewing?

@nizarmah If I just run make lms-static or similar on my devstack, will that test this? I just want some way of confirming that the "normal" process to build static files will still work correctly and that TinyMCE will work for everyone after this change. I looked at the code itself and it looks good.

[nizarmah]

@bradenmacdonald I'm glad you mentioned that, it turns out I forgot to update the build instructions.
Those changes won't get tested by running make lms-static alone. To test this, you'll need to follow the common/static/js/vendor/tinymce/BUILD_README.txt.

I know I haven't linked a sandbox above, but we can use the one from https://github.com/edx/edx-platform/pull/25695 👍🏼

Here's how I'd personally test it in order to verify that the TinyMCE editor will work for everyone after this change.

Switching Back to Default TinyMCE Editor

Removing ADSK Plugin

1. Change user to edxapp
   sudo -Hu edxapp bash
2. Activate the virtual environment, just in case you need it.
   cd && . edxapp_env && . venvs/edxapp/bin/activate
3. Change directory to where you'll rebuild the files
   cd ~/edx-platform/common/static/js/vendor/tinymce
4. Remove the ADSK Plugin
   rm -rf js/tinymce/plugins/adsklink

Rebuilding TinyMCE Editor

1. Change user to edxapp
   sudo -Hu edxapp bash
2. Activate the virtual environment, just in case you need it.
   cd && . edxapp_env && . venvs/edxapp/bin/activate
3. Change directory to where you'll rebuild the files
   cd ~/edx-platform/common/static/js/vendor/tinymce
4. You won't need to re-install things here, because the configuration changes from https://github.com/edx/configuration/pull/6179 did already :)
5. Clean up the JS files that exist already
   npx jake clean-js
6. Rebuild the themes and plugins with the default ones:
   npx jake minify bundle[themes:*,plugins:*]
7. Exit the edxapp user
8. Edit the environment settings and comment out JS_ENV_EXTRA_CONFIG and its content
   sudo nano /edx/etc/studio.yml
9. Something that I advise to do, but isn't necessary is to delete all existing static files (so that if there's a problem, it will show up)
   sudo rm -rf /edx/var/edxapp/staticfiles/*
10. Rebuild the static assets
    /edx/bin/edxapp-update-assets
11. Restart all services (especially CMS)
    /edx/bin/supervisorctl restart all

Once that's done, go to the TinyMCE editor and verify that the ADSK Plugin is no longer being used.

To verify that everything is working normally, you can compare the result to this sandbox which is a default build without rebuilt TinyMCE editor:

• LMS: https://configpr6179.sandbox.opencraft.hosting/
• Studio: https://studio.configpr6179.sandbox.opencraft.hosting/

Use staff@example.com/edx to login in.

Adding the ADSK Plugin Again

Adding ADSK Plugin Files

1. Change user to edxapp
   sudo -Hu edxapp bash
2. Activate the virtual environment, just in case you need it.
   cd && . edxapp_env && . venvs/edxapp/bin/activate
3. Change directory to where you'll rebuild the files
   cd ~/edx-platform/common/static/js/vendor/tinymce
4. Copy the ADSK Plugin Back
   cp -r adsklink js/tinymce/plugins/

Rebuilding TinyMCE Editor

Follow the steps mentioned above in order to rebuild the files, but for step 8 uncomment the JS_ENV_EXTRA_CONFIG setting and its content instead.

Now, open the TinyMCE editor again, and you should have the ADSK Plugin available to use.

[bradenmacdonald]

👍 Thanks for figuring this out, and for the documentation update, and the very explicit test instructions (just what I needed)! 💯

• I tested this: as described, in PR description and comment
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation

Please squash to one commit and I'll get this merged.

[nizarmah]

@bradenmacdonald I'm glad the testing instructions were helpful 😃

I have squashed. The commit message was mainly about the changes in the commit, because the title of this Pull Request is a bit vague...

Let me know if you'd like me to change that though. 👍🏼

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[openedx-webhooks]

@nizarmah 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[ormsbee]

Thank you for the contribution (and thank you @bradenmacdonald for the review)! The PR message for this gives a fantastic level of detail that will undoubtedly be very useful for people who are debugging pipeline issues years down the line. Please feel free to add more of those details to the commit messages on any future issues.

Thanks again!

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.