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

Upgrades Node version 6.11.0 -> 10.15.1 #179

Merged
merged 4 commits into from
Feb 22, 2019
Merged

Conversation

conorsch
Copy link
Contributor

Bumps the nodejs version to a supported LTS. The actual deploy logic must be updated separately—first, let's validate no breakage to the best of our ability here.

For the most part, I've abstained from updating the node dependencies themselves, opting to bump only the parent nodejs/npm versions. The exception is to the jade dep; see commit messages for details there.

Testing

First, it's critical that you run docker-compose build (or docker-compose up --build) on this branch. otherwise, you'll still be using the old containers, even if you have this branch checked out! See output from docker-compose for reference:

WARNING: Image for service node was built because it did not already exist. To rebuild this image you must use docker-compose build or docker-compose up --build.

Fire up the dev env, interact with the local site manually, see if you can identify any breakage. Keep an eye on the dev console in particular.

Refs: https://github.com/freedomofpress/fpf-www-projects/issues/33

Conor Schaefer added 4 commits February 13, 2019 17:55
The dynamically generated dockerfile logic is acceptable, for now, but
we want to ensure that the config we're using is version controlled, so
let's commit the output from the generate makefile targets.
The 4.x node series is EOL, so transitioning to the 10.x series, which
has LTS support until 2021-04. See details:

https://github.com/nodejs/Release#release-schedule
Given the node version bump from 4.x -> 10.x, we can now commit the
exact node versions we expect to be installed via package.json. Allows
us to run e.g. `npm audit` in CI to analyize node dependencies for
security vulnerabilities; deferring that addition for now, pending a
full audit.
Prior to doing so, the npm action was throwing an error:

node_1        | internal/modules/cjs/loader.js:583
node_1        |     throw err;
node_1        |     ^
node_1        |
node_1        | Error: Cannot find module 'jade'
node_1        |     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
node_1        |     at Function.Module._load (internal/modules/cjs/loader.js:507:25)
node_1        |     at Module.require (internal/modules/cjs/loader.js:637:17)
node_1        |     at require (internal/modules/cjs/helpers.js:22:18)
node_1        |     at Object.<anonymous> (/var/www/django/node_modules/jadeify/lib/jadeify.js:4:12)
node_1        |     at Module._compile (internal/modules/cjs/loader.js:689:30)
node_1        |     at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
node_1        |     at Module.load (internal/modules/cjs/loader.js:599:32)
node_1        |     at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
node_1        |     at Function.Module._load (internal/modules/cjs/loader.js:530:3)

Ran 'npm install jade' to get it into the package declaration. During
install, npm emitted a warning that "jade" has been renamed to "pug",
and recommended installing the latest pug. Deferring that change pending
a full audit of the node deps.
Copy link
Contributor

@harrislapiroff harrislapiroff left a comment

Choose a reason for hiding this comment

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

Same issues as freedomofpress/securedrop.org#598

Again, approving provisionally.

@harrislapiroff
Copy link
Contributor

Copying over my comments from freedomofpress/securedrop.org#598:

I actually noticed, again, that the optional deps do switch around when I run docker-compose up --build after deleting my node_modules file. However, it looks like this is a known issue in npm that might, if I understand this issue, be resolved in an upcoming version of npm. Given that, I'm happy to merge Node upgrade PRs and just bear it till then.

@harrislapiroff harrislapiroff merged commit 32ec29b into master Feb 22, 2019
@harrislapiroff harrislapiroff deleted the upgrade-node-to-10.x branch February 22, 2019 16:23
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.

2 participants