Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

bug 1468491: Update node 8, dependencies #4852

Merged
merged 10 commits into from
Jul 26, 2018
Merged

Conversation

jwhitlock
Copy link
Contributor

@jwhitlock jwhitlock commented Jun 15, 2018

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

@jwhitlock jwhitlock changed the title WIP bug 1468491: Update node 8, dependencies bug 1468491: Update node 8, dependencies Jun 15, 2018
@jwhitlock jwhitlock force-pushed the node-8-1468491 branch 3 times, most recently from 6e1ef05 to d103dc4 Compare June 18, 2018 12:42
@codecov-io
Copy link

codecov-io commented Jun 18, 2018

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor Author

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:

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

I still need to:

@jwhitlock jwhitlock force-pushed the node-8-1468491 branch 5 times, most recently from 2d8359f to 7aa7249 Compare July 17, 2018 19:37
@jwhitlock
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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
Copy link

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

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

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!

@@ -148,5 +148,10 @@ shell_plus: up
lint:
flake8 kuma docs tests

npmrefresh:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice convenience.

On your local (host) machine, open a new shell and run from the root of the
Kuma repository::

,/node_modules/.bin/gulp
Copy link
Contributor

Choose a reason for hiding this comment

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

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


When doing front-end development on your local machine, run the following in its
own shell from the root directory of your local Kuma repository::
make build-static
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

package.json Outdated
},
"dependencies": {
"clean-css": "3.4.28",
"fibers": "2.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

😄 I suspect you're right.

@jwhitlock
Copy link
Contributor Author

jwhitlock commented Jul 25, 2018

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 ✅

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

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
Copy link

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

jwhitlock and others added 10 commits July 26, 2018 08:16
These packages are unused in the asset toolchain or other development
processes.
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
* Add another signing key

The kuma_base py3 variant can't be updated, because node-sass requires a
version of node-gyp that doesn't support Python 3.6.
* node-sass 4.3.0 → 4.9.2: CSS Grid support, supports node v8-v10
  The generated CSS appears identical except for whitespace in the
  non-compressed version.
* uglify-js 2.4.13 → 2.8.29: Last of the v2 API/CLI. There are some
  changes, such as extra spaces, quoting for objects using keywords as
  keys, bracketing of one-liners in loops, and trailing newlines on files.
* stylelint 7.10.1 → 9.2.1: Drop node v4, new semvar policy
* clean-css 3.4.23 → 3.4.28: Latest 3.4 update
  There is a 4.x series that breaks asset processing (first error is that
  django-pipeline can't find a font file).

With the updated node tools, the py3 variant can build with node 8.
In the docker image, use "npm install" to install the versions in
package.json to /tools, and symlink the binaries to /usr/local/bin.
The node packages used in build process are in dependencies, and
installed with --production.

npm 5, bundled with node 8, adds package-lock.json as an improved
npm-shrinkwrap.json.

To make it easier to update npm pacakges, the user is set to 1000 (kuma
in the container), and folders and files are owned by kuma. There is a
new --chown parameter to COPY in Dockerfile, and this is used to avoid
having to chown files after copying.

In the TravisCI builds, install tools using "npm install". Rename
INSTALL_PIPELINE to INSTALL_NPM_TOOLS to reflect new usage.

The tox stylelint target is renamed "assetlint", so that we can move
more style checks to TravisCI.
Rewrite front-end development sections to add an overview of asset
processing in development and production modes, the limitations of host
processing with gulp, and how to run linters. Mark the gulp installation
as optional.

Add a Makefile command and instructions for regenerating
package-lock.json.

Change section titles to MDN standard of sentence case.
Move eslint from devDependencies to dependencies, and run against pull
requests in TravisCI.
* stylelint 9.2.1 → 9.3.0: Support XML, fix some false positives
eslint 3.19.0 → 5.1.0: Over a year of updates. New indentation rules,
and detection for unnecessary escapes, applied to affected JS files.
Includes removal of shelljs dependeny, which has a security issue.
* 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
Running a fresh npm install makes some changes to package-lock.json
@jwhitlock
Copy link
Contributor Author

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 ✅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants