bug 1468491: Update node 8, dependencies

[jwhitlock]

This is a bigger change than I'd like, but there's a lot of stuff about the asset workflow that hasn't been touched for over a year, and an incremental series of PRs wasn't working. It includes the version updates from PR #4781.

Update from node 6.x to 8.x, copying the installation steps from the official node Dockerfile. Changes:

• Drop the xz-utils package
• Add a node user
• Give the kuma user a home directory and ownership of files
• Use the new COPY --chown=kuma:kuma syntax to set file ownership
• Add another signing key
• Install node.js tools to /tools via npm install --production, and symlink command line tools into /usr/local/bin.

Update the front-end development documentation to describe the new package.json workflow, and to be clearer about development vs. production assets, and Docker-based asset compiling vs host-based gulp.

Node package updates:

• node-sass 4.3.0 → 4.9.2: CSS Grid support, supports node v8-v10
• uglify-js 2.4.13 → 2.8.29: Last of the v2 API/CLI
• styleline 7.10.1 → 9.3.0: Drop node v4, adopt semvar policy
• clean-css 3.4.23 → 3.4.28: Latest 3.4 update, 4.x breaks processing

From PR #4781:

• eslint 3.19.0 → 5.1.0: Some new rules, applied to Kuma JS files
• gulp-sass 3.1.0 → 4.0.1: Drop support for Node < 6, replace gulp-utils
• gulp-stylelint 3.9.0 → 7.0.0: Require stylelint ^9.0.0 as peer
• gulp-watch 4.3.6 → 5.0.0: Support node v7 and later
• husky 0.13.3 → 0.14.3: Drop node 0.12 support

Dropped node packages which appear unused:

• csslint
• fibers
• jshint

[codecov-io]

Codecov Report

Merging #4852 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4852   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files         271      271           
  Lines       24073    24073           
  Branches     1747     1747           
=======================================
  Hits        23056    23056           
  Misses        804      804           
  Partials      213      213

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a3f945...c7cc405. Read the comment docs.

[jwhitlock]

The snyk warning is only visible to one person at a time 😞 . I've contacted support to see if there is a fix for the GitHub integration. If there's not, I may remove the snyk integration and move the security check to a TravisCI test.

The issues are:

• (Medium) Uninitialized Memory Exposure in tunnel-agent from tunnel-agent via node-sass. I believe this is fixed in node-sass 4.9.1, and I'm using this as a test case for how to update package.json.
• (Low) Prototype Pollution from hoek via a few packages. This was partially fixed in node-sass 4.9.1, but we need a fixed release for node-gyp as well - bump request to 2.87.0 nodejs/node-gyp#1492
• (Low) Regular Expression Denial of Service (ReDoS) from braces via stylelint 9.2.1. It is possible a stylelint update will fix, I haven't investigated.
• (Low) Regular Expression Denial of Service (ReDoS) from clean-css 3.4.28. It is possible that an upgrade to the 4.x series will fix, but may require code changes as well.

None of these seem like blockers to me, if the minor version updates don't make them go away.

I still need to:

• Write some initial documentation on updating package.json and npm-shrinkwrap.json, similar to the Kumascript README.
• Try host-native tools, such as Front-end Development and Compiling Sass files, make sure they still work.
• Update host-native tools, from PR deps(node): update to the latest version of dependencies #4781
• (Maybe) Include dev packages in npm-shrinkwrap.json
• Update docs, the comments in .env-dist.dev for the default UID change
• Verify that asset file changes are cosmetic (whitespace, swapped attributes, etc.)

[jwhitlock]

I think this is ready to review. I'm going to take a closer look at the assets generated by the current master vs this branch, and will report results and commands, but I don't expect any further commits.

It made more sense to include the devDependencies in npm-shrinkwrap.json, so they are in there too.

I've added eslint to the TravisCI checks, so that will be checked with each PR. I'd like to remove husky and the precommit hook, but I can wait for a different PR.

[schalkneethling]

@jwhitlock

It made more sense to include the devDependencies in npm-shrinkwrap.json

Do we still need this with Nodejs 8+? Is that not replaced by package-lock?

I'd like to remove husky and the precommit hook, but I can wait for a different PR.

Any specific reason to remove the pre-commit hook? I find it very useful in general but, specifically if one has people contributing to the codebase. It avoids some simple issues one can easily overlook that then slows down the PR review process.

Thanks for working on this John!

[jwhitlock]

I read up on package-lock.json (starting at https://stackoverflow.com/q/44258235/10612), and it looks like it would work just as well for us. I've added a commit to switch to that from npm-shrinkwrap.json.

I seldom write any JS or CSS, but the pre-commit hook adds about a second to every commit. It adds up when I'm doing a rebase. For this PR, when I was updating the linters, rebasing failed repeatedly because the accepted style changed.

I'd prefer to just run the linters in TravisCI, and committers be able to re-run the linters locally to verify their fixes, However, we could also add instructions for disabling the pre-commit hook.

[schalkneethling]

However, we could also add instructions for disabling the pre-commit hook.

@jwhitlock I would kinda prefer this option. Basically people need to append --no-verify to the end of their commit command. Does that work for you all? @escattone

[escattone]

I went through this carefully, partly just because I wanted to refresh my understanding of our use of Node within Kuma. This is a really nice step forward. I'm requesting two changes:

• Remove fibers from the dependencies.
• Documentation nit

I personally don't like running the stylelint and eslint linters on all of the relevant files for every commit, and would prefer to keep that as a separate step that can be run locally from time to time and for sure as a pre-condition for the pull-request, but as @jwhitlock said, I don't often commit JS or CSS files, so I'll defer to @schalkneethling.

Thanks for all of your work on this @jwhitlock!

[escattone]

This is a nice convenience.

[escattone]

Typo: replace the comma with a period, so ./node_modules/.bin/gulp instead of ,/node_modules/.bin/gulp.

[escattone]

Just for the record, this doesn't work for me when DEBUG=False (e.g., none of the CSS files are hashed) , but that was the case prior to this PR. When DEBUG=False, I have to use ./manage.py collectstatic or DJANGO_SETTINGS_MODULE=kuma.settings.prod make build-static to actually generate the production assets. I think this issue is related to export DJANGO_SETTINGS_MODULE ?= kuma.settings.testing in the Makefile, but that's for another PR. Just wanted to note it here.

[jwhitlock]

Hmm that seems to belong in the Generating Production Assets section.

[escattone]

As far as I can tell, fibers is not used/needed (even as a dependency), and we should remove it. I tested this by removing it from package.json, re-generating the package-lock.json, re-building the kuma_base image, bringing-up a fresh docker-compose (down then up -d), and then testing everything again(npm run stylelint, npm run eslint, make build-static, DJANGO_SETTINGS_MODULE=kuma.settings.prod make build-static, etc.). All of the node tools we use in Kuma worked fine without it.

Also, now that we're on Node.js 8 and the nice new world of async and await, I don't think we should encourage anyone to reach for fibers, which is most often used as a way to write asynchronous code that looks like it's synchronous.

[jwhitlock]

Now I'm curious how fibers got into the environment in the first place. Not curious enough to chase it down, I assume we copy/pasted from the KumaScript initialization.

[escattone]

😄 I suspect you're right.

[jwhitlock]

Added a few more commits:

• There was a typo in the kuma image Dockerfile that prevented it from building hashed assets. This is fixed.
• Addressed the ,/node_modules vs ./node_modules documentation nit
• Dropped fibers, since we don't seem to use it. It looks like it was bundling a bunch of libraries that are now installed with npm, so they show up in package-lock.json as resolved with integrity rather than bundled.
• Adjusted the command to go to "production-mode", so that hashed assets will be built.

I rebased on master as well, which makes the commands to check the generated assets a bit easier. On a MacOS box with opendiff from Xcode:

mkdir compare_static
git fetch
git checkout master
git pull
VERSION=latest make build-base build-kuma
docker run -i -v ${PWD}/compare_static:/artifacts quay.io/mozmar/kuma:latest cp -a /app/static /artifacts/old

git checkout node-8-1468491
VERSION=latest make build-base build-kuma
docker run -i -v ${PWD}/compare_static:/artifacts quay.io/mozmar/kuma:latest cp -a /app/static /artifacts/new

cd compare_static
opendiff old new

I then used diff -b (ignore whitespace) and cmp (show column of first difference) to look at the specific changes. The differences are:

• The new uglifyjs add a newline to the end of JavaScript files, which is carried all the way to the hashed minified JS
• build/js/ckeditor-prod.js has a JS object with a JS keyword as a key like {var: 1}. The old uglifyjs outputs this as {"var": 1}, and the new uglifyjs as {var: 1}. build/js/syntax-prism.js gets similar treatment for {function: /[-a-z0-9]+(?=\()/i}, and build/js/wiki-compat-tables.js with class.
• The old uglifyjs formats a one-statement do-while loop as do x;while (condition), the new uglifyjs does do{x}while (condition) (CKeditor again)
• The old uglifyjs breaks lines at column 32,767 (2 ^ 15), and the new uglifyjs at column 32,000 (a weird number), affecting jquery-ui.js, main.js, other big files.
• The new eslint complained about "unneeded escaped characters", and these changes are preserved in the minified JS.
• The old node-sass put a new line after every selector in a multi-selector rule (.warning,\n.obsolete,\n), and then new cleancss sometimes puts multiple selectors on the same line (.warning, .obsolete,\n). These whitespace changes are eliminated in the minified CSS.

I'm convinced the new tools put out functionally the same code as the old tools ✅

[escattone]

Thanks for the changes, and really nice work on verifying that the production assets had not substantially changed! Also, good catch on the ENV command in the kuma Dockerfile. There's a lot in the PR, thanks for the great work @jwhitlock!

[schalkneethling]

Indeed, thanks a lot for this @jwhitlock. This is big step forward. 👍

[jwhitlock]

Force-push to rebase on master, and to re-arrange and combine commits, bringing them from 22 to 11 commits. This included:

• Moving removal of csslint, fibers, and jshint to the top, and dropping the commits that update them
• Starting with package-lock.json, rather than npm-shrinkwrap.json
• Moving the fixes for documentation typos and updates into the initial commit.

I'll merge when CI gives a ✅