Merge pull request #325 from heroku/extended-warnings

Extended warnings

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - Do not fail build on output errors
 - Do not prune before install (shrinkwrap unsupported by prune)
+- Extended warnings (missing dependencies, econnreset, no start)
 
 ## v90 (2016-4-20)
 

bin/compile

@@ -42,6 +42,8 @@ handle_failure() {
   header "Build failed"
   warn_untracked_dependencies "$LOG_FILE"
   warn_angular_resolution "$LOG_FILE"
+  warn_missing_devdeps "$LOG_FILE"
+  warn_econnreset "$LOG_FILE"
   failure_message | output "$LOG_FILE"
 }
 trap 'handle_failure' ERR
@@ -158,3 +160,6 @@ summarize_build() {
 
 header "Build succeeded!"
 summarize_build | output "$LOG_FILE"
+
+warn_no_start "$LOG_FILE"
+warn_unmet_dep "$LOG_FILE"

lib/binaries.sh

@@ -18,7 +18,10 @@ install_nodejs() {
 
   echo "Downloading and installing node $version..."
   local download_url="https://s3pository.heroku.com/node/v$version/node-v$version-$os-$cpu.tar.gz"
-  curl "$download_url" --silent --fail  --retry 5 --retry-max-time 15 -o /tmp/node.tar.gz || (echo "Unable to download node $version; does it exist?" && false)
+  local code=$(curl "$download_url" --silent --fail --retry 5 --retry-max-time 15 -o /tmp/node.tar.gz --write-out "%{http_code}")
+  if [ "$code" != "200" ]; then
+    echo "Unable to download node $version; does it exist?" && false
+  fi
   tar xzf /tmp/node.tar.gz -C /tmp
   rm -rf $dir/*
   mv /tmp/node-v$version-$os-$cpu/* $dir

lib/failure.sh

@@ -35,6 +35,14 @@ warning() {
   echo "" >> $warnings
 }
 
+warn() {
+  local tip=${1:-}
+  local url=${2:-https://devcenter.heroku.com/articles/nodejs-support}
+  echo " !     $tip" || true
+  echo "       $url" || true
+  echo ""
+}
+
 warn_node_engine() {
   local node_engine=${1:-}
   if [ "$node_engine" == "" ]; then
@@ -49,7 +57,7 @@ warn_node_engine() {
 warn_prebuilt_modules() {
   local build_dir=${1:-}
   if [ -e "$build_dir/node_modules" ]; then
-    warning "node_modules checked into source control" "https://docs.npmjs.com/misc/faq#should-i-check-my-node-modules-folder-into-git"
+    warning "node_modules checked into source control" "https://blog.heroku.com/node-habits-2016#9-only-git-the-important-bits"
   fi
 }
 
@@ -87,3 +95,43 @@ warn_angular_resolution() {
     warning "Bower may need a resolution hint for angular" "https://github.com/bower/bower/issues/1746"
   fi
 }
+
+warn_missing_devdeps() {
+  local log_file="$1"
+  if grep -qi 'cannot find module' "$log_file"; then
+    warning "A module may be missing from package.json" "https://devcenter.heroku.com/articles/troubleshooting-node-deploys#ensure-you-aren-t-relying-on-untracked-dependencies"
+    if [ "$NPM_CONFIG_PRODUCTION" == "true" ]; then
+      local devDeps=$(read_json "$BUILD_DIR/package.json" ".devDependencies")
+      echo "devDependencies: $devDeps"
+      if [ "$devDeps" != "" ]; then
+        warning "A module may be specified in devDependencies instead of dependencies" "https://devcenter.heroku.com/articles/nodejs-support#devdependencies"
+      fi
+    fi
+  fi
+}
+
+warn_no_start() {
+  local log_file="$1"
+  if ! [ -e "$BUILD_DIR/Procfile" ]; then
+    local startScript=$(read_json "$BUILD_DIR/package.json" ".scripts.start")
+    if [ "$startScript" == "" ]; then
+      if ! [ -e "$BUILD_DIR/server.js" ]; then
+        warn "This app may not specify any way to start a node process" "https://devcenter.heroku.com/articles/nodejs-support#default-web-process-type"
+      fi
+    fi
+  fi
+}
+
+warn_econnreset() {
+  local log_file="$1"
+  if grep -qi 'econnreset' "$log_file"; then
+    warning "ECONNRESET issues may be related to npm versions" "https://github.com/npm/registry/issues/10#issuecomment-217141066"
+  fi
+}
+
+warn_unmet_dep() {
+  local log_file="$1"
+  if grep -qi 'unmet dependency' "$log_file" || grep -qi 'unmet peer dependency' "$log_file"; then
+    warn "Unmet dependencies don't fail npm install but may cause runtime issues" "https://github.com/npm/npm/issues/7494"
+  fi
+}

test/fixtures/econnreset-mock/package.json

+{
+  "name": "econnreset-mock",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "start": "node server.js",
+    "postinstall": "echo 'npm ERR! network read ECONNRESET' && exit 1"
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC"
+}

test/fixtures/invalid-node/package.json

@@ -9,6 +9,6 @@
   "dependencies": {
   },
   "engines": {
-    "node": "0.11.33"
+    "node": "0.11.333"
   }
 }

test/fixtures/missing-devdeps-1/package.json

+{
+  "name": "missing-devdeps-1",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "postinstall": "node postinstall.js"
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC"
+}

test/fixtures/missing-devdeps-1/postinstall.js

+const lodash = require('lodash');

test/fixtures/missing-devdeps-2/package.json

+{
+  "name": "missing-devdeps-1",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "postinstall": "node postinstall.js"
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC",
+  "devDependencies": {
+    "lodash": "4.13.1"
+  }
+}

test/fixtures/missing-devdeps-2/postinstall.js

+const lodash = require('lodash');

test/fixtures/no-start/package.json

+{
+  "name": "no-start",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC"
+}

test/fixtures/no-version/package.json

@@ -5,5 +5,8 @@
   "repository" : {
     "type" : "git",
     "url" : "http://github.com/example/example.git"
+  },
+  "scripts": {
+    "start": "node server.js"
   }
 }

test/fixtures/unmet-dep/package.json

+{
+  "name": "unmet-dep",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "start": "node server.js"
+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC",
+  "dependencies": {
+    "grunt-steroids": "0.2.3"
+  }
+}

test/run

 #!/usr/bin/env bash
 # See README.md for info on running these tests.
 
+testWarnUnmetDep() {
+  compile "unmet-dep"
+  assertCaptured "may cause runtime issues"
+  assertCapturedSuccess
+  compile "no-version"
+  assertNotCaptured "may cause runtime issues"
+  assertCapturedSuccess
+}
+
+testWarnEconnreset() {
+  compile "econnreset-mock"
+  assertCaptured "may be related to npm versions"
+  assertCapturedError
+  compile "no-version"
+  assertNotCaptured "may be related to npm versions"
+  assertCapturedSuccess
+}
+
+testWarnNoStart() {
+  compile "no-start"
+  assertCaptured "may not specify any way to start"
+  assertCapturedSuccess
+  compile "no-version"
+  assertNotCaptured "may not specify any way to start"
+  assertCapturedSuccess
+}
+
+testWarnDevDeps() {
+  compile "missing-devdeps-1"
+  assertCaptured "A module may be missing"
+  assertNotCaptured "A module may be specified"
+  assertCapturedError
+  compile "missing-devdeps-2"
+  assertCaptured "A module may be missing"
+  assertCaptured "A module may be specified"
+  assertCapturedError
+  compile "failing-build"
+  assertNotCaptured "A module may be missing"
+  assertNotCaptured "A module may be specified"
+  assertCapturedError
+}
+
 testEnvBlacklist() {
   local cache=$(mktmpdir)
   local env_dir=$(mktmpdir)
@@ -159,8 +201,8 @@ testConcurrencySaneMaximum() {
 
 testInvalidNode() {
   compile "invalid-node"
-  assertCaptured "Downloading and installing node 0.11.33"
-  assertCaptured "Unable to download node 0.11.33"
+  assertCaptured "Downloading and installing node 0.11.333"
+  assertCaptured "Unable to download node 0.11.333"
   assertCapturedError
 }