From 1e5fb28ea616c15b6626986f38096f991a468dd4 Mon Sep 17 00:00:00 2001 From: Christian Medina Date: Sun, 24 Feb 2019 18:32:49 +0000 Subject: [PATCH 1/2] refactor: centralize error message logging --- lib/install.js | 17 +++-------------- lib/utils/error-handler.js | 7 ++----- lib/utils/log-error-message.js | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 19 deletions(-) create mode 100644 lib/utils/log-error-message.js diff --git a/lib/install.js b/lib/install.js index e15bc47919100..980b3728fb34d 100644 --- a/lib/install.js +++ b/lib/install.js @@ -119,6 +119,7 @@ var unlock = locker.unlock var parseJSON = require('./utils/parse-json.js') var output = require('./utils/output.js') var saveMetrics = require('./utils/metrics.js').save +var logErrorMessage = require('./utils/log-error-message.js') // install specific libraries var copyTree = require('./install/copy-tree.js') @@ -342,13 +343,7 @@ Installer.prototype.run = function (_cb) { if (installEr) self.failing = true chain(postInstallSteps, function (postInstallEr) { if (installEr && postInstallEr) { - var msg = errorMessage(postInstallEr) - msg.summary.forEach(function (logline) { - log.warn.apply(log, logline) - }) - msg.detail.forEach(function (logline) { - log.verbose.apply(log, logline) - }) + logErrorMessage(postInstallEr, 'warn', 'verbose') } cb(installEr || postInstallEr, self.getInstalledModules(), self.idealTree) }) @@ -736,13 +731,7 @@ Installer.prototype.printWarnings = function (cb) { if (warning.code === 'EPACKAGEJSON' && self.global) return if (warning.code === 'ENOTDIR') return warned = true - var msg = errorMessage(warning) - msg.summary.forEach(function (logline) { - log.warn.apply(log, logline) - }) - msg.detail.forEach(function (logline) { - log.verbose.apply(log, logline) - }) + logErrorMessage(warning, 'warn', 'verbose') }) if (warned && log.levels[npm.config.get('loglevel')] <= log.levels.warn) console.error() cb() diff --git a/lib/utils/error-handler.js b/lib/utils/error-handler.js index ba9d9f8e252e5..bba7a43a119e6 100644 --- a/lib/utils/error-handler.js +++ b/lib/utils/error-handler.js @@ -12,7 +12,7 @@ var exitCode = 0 var rollbacks = npm.rollbacks var chain = require('slide').chain var writeFileAtomic = require('write-file-atomic') -var errorMessage = require('./error-message.js') +var logErrorMessage = require('./log-error-message.js') var stopMetrics = require('./metrics.js').stop var mkdirp = require('mkdirp') var fs = require('graceful-fs') @@ -198,10 +198,7 @@ function errorHandler (er) { if (v) log.error(k, v) }) - var msg = errorMessage(er) - msg.summary.concat(msg.detail).forEach(function (errline) { - log.error.apply(log, errline) - }) + var msg = logErrorMessage(er, 'error', 'error') if (npm.config && npm.config.get('json')) { var error = { error: { diff --git a/lib/utils/log-error-message.js b/lib/utils/log-error-message.js new file mode 100644 index 0000000000000..b82838de94acf --- /dev/null +++ b/lib/utils/log-error-message.js @@ -0,0 +1,16 @@ +'use strict' +var log = require('npmlog') +var errorMessage = require('./error-message.js') + +module.exports = logErrorMessage + +function logErrorMessage (er, summaryLevel, detailLevel) { + var msg = errorMessage(er) + msg.summary.forEach(function (logline) { + log[summaryLevel].apply(log, logline) + }) + msg.detail.forEach(function (logline) { + log[detailLevel].apply(log, logline) + }) + return msg +} From c48fc7c201851994245e78a782461b8bac380d29 Mon Sep 17 00:00:00 2001 From: Christian Medina Date: Sun, 24 Feb 2019 19:59:00 +0000 Subject: [PATCH 2/2] install: log failed optional platform dependencies as info --- lib/install/report-optional-failure.js | 9 ++++++++- test/tap/full-warning-messages.js | 22 +++++++++++++--------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/lib/install/report-optional-failure.js b/lib/install/report-optional-failure.js index 41c7c535bdd63..dec7f2e6a3d7f 100644 --- a/lib/install/report-optional-failure.js +++ b/lib/install/report-optional-failure.js @@ -1,6 +1,7 @@ 'use strict' var path = require('path') var moduleName = require('../utils/module-name.js') +var logErrorMessage = require('../utils/log-error-message.js') module.exports = reportOptionalFailure @@ -27,5 +28,11 @@ function reportOptionalFailure (tree, what, error) { error.optional = id error.location = location - topTree.warnings.push(error) + if (error.code === 'EBADPLATFORM') { + // Optional dependencies irrelevant to the current OS probably aren't worth + // always warning users about, so indicate this issue at a lower log level. + logErrorMessage(error, 'info', 'verbose') + } else { + topTree.warnings.push(error) + } } diff --git a/test/tap/full-warning-messages.js b/test/tap/full-warning-messages.js index c986695d67e99..d55f81fcae9b9 100644 --- a/test/tap/full-warning-messages.js +++ b/test/tap/full-warning-messages.js @@ -70,7 +70,7 @@ function exists (t, filepath, msg) { t.pass(msg) return true } catch (ex) { - t.fail(msg, {found: null, wanted: 'exists', compare: 'fs.stat(' + filepath + ')'}) + t.fail(msg, { found: null, wanted: 'exists', compare: 'fs.stat(' + filepath + ')' }) return false } } @@ -78,7 +78,7 @@ function exists (t, filepath, msg) { function notExists (t, filepath, msg) { try { fs.statSync(filepath) - t.fail(msg, {found: 'exists', wanted: null, compare: 'fs.stat(' + filepath + ')'}) + t.fail(msg, { found: 'exists', wanted: null, compare: 'fs.stat(' + filepath + ')' }) return true } catch (ex) { t.pass(msg) @@ -87,25 +87,29 @@ function notExists (t, filepath, msg) { } test('tree-style', function (t) { - common.npm(['install', '--json', '--loglevel=warn'], {cwd: base}, function (err, code, stdout, stderr) { + common.npm(['install', '--json', '--loglevel=info'], { cwd: base }, function (err, code, stdout, stderr) { if (err) throw err t.is(code, 0, 'result code') var result = JSON.parse(stdout) t.is(result.added.length, 1, 'only added one module') t.is(result.added[0].name, 'modA', 'modA got installed') - t.is(result.warnings.length, 1, 'one warning') - var stderrlines = stderr.trim().split(/\n/) - t.is(stderrlines.length, 2, 'two lines of warnings') - t.match(stderr, /SKIPPING OPTIONAL DEPENDENCY/, 'expected optional failure warning in stderr') + t.is(result.warnings.length, 0, 'no warnings') + t.match(stderr, /SKIPPING OPTIONAL DEPENDENCY/, 'expected optional failure info in stderr') t.match(stderr, /Unsupported platform/, 'reason for optional failure in stderr') - t.match(result.warnings[0], /SKIPPING OPTIONAL DEPENDENCY/, 'expected optional failure warning in JSON') - t.match(result.warnings[0], /Unsupported platform/, 'reason for optional failure in JSON') exists(t, modJoin(base, 'modA'), 'module A') notExists(t, modJoin(base, 'modB'), 'module B') t.done() }) }) +test('tree-style', function (t) { + common.npm(['install', '--loglevel=warn'], { cwd: base }, function (err, code, stdout, stderr) { + if (err) throw err + t.is(stderr.length, 0, 'EBADPLATFORM errors for optional deps are excluded from warnings') + t.done() + }) +}) + test('cleanup', function (t) { cleanup() t.end()