Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils: [Fix] report the error stack on a resolution error #599

Merged
merged 1 commit into from
Jan 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/files/load-error-resolver.js
Original file line number Diff line number Diff line change
@@ -1 +1 @@
throw new Error('TEST ERROR')
throw new SyntaxError('TEST SYNTAX ERROR')
9 changes: 7 additions & 2 deletions tests/src/core/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ import * as fs from 'fs'
import * as utils from '../utils'

describe('resolve', function () {
// We don't want to test for a specific stack, just that it was there in the error message.
function replaceErrorStackForTest(str) {
return typeof str === 'string' ? str.replace(/(\n\s+at .+:\d+\)?)+$/, '\n<stack-was-here>') : str
}

it('throws on bad parameters', function () {
expect(resolve.bind(null, null, null)).to.throw(Error)
})
Expand Down Expand Up @@ -60,7 +65,7 @@ describe('resolve', function () {
, Object.assign({}, testContext, { getFilename: function () { return utils.getFilename('exception.js') } }),
)).to.equal(undefined)
expect(testContextReports[0]).to.be.an('object')
expect(testContextReports[0].message).to.equal('Resolve error: foo-bar-resolver-v2 resolve test exception')
expect(replaceErrorStackForTest(testContextReports[0].message)).to.equal('Resolve error: foo-bar-resolver-v2 resolve test exception\n<stack-was-here>')
expect(testContextReports[0].loc).to.eql({ line: 1, column: 0 })

testContextReports.length = 0
Expand Down Expand Up @@ -153,7 +158,7 @@ describe('resolve', function () {
, Object.assign({}, testContext, { getFilename: function () { return utils.getFilename('exception.js') } }),
)).to.equal(undefined)
expect(testContextReports[0]).to.be.an('object')
expect(testContextReports[0].message).to.equal('Resolve error: TEST ERROR')
expect(replaceErrorStackForTest(testContextReports[0].message)).to.equal('Resolve error: SyntaxError: TEST SYNTAX ERROR\n<stack-was-here>')
expect(testContextReports[0].loc).to.eql({ line: 1, column: 0 })
})

Expand Down
3 changes: 3 additions & 0 deletions utils/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

### Fixed
- Uses createRequireFromPath to resolve loaders ([#1591], thanks [@arcanis])
- report the error stack on a resolution error ([#599], thanks [@sompylasar])

## v2.5.0 - 2019-12-07

Expand Down Expand Up @@ -72,6 +73,7 @@ Yanked due to critical issue with cache key resulting from #839.
[#1166]: https://github.com/benmosher/eslint-plugin-import/issues/1166
[#1160]: https://github.com/benmosher/eslint-plugin-import/pull/1160
[#1035]: https://github.com/benmosher/eslint-plugin-import/issues/1035
[#599]: https://github.com/benmosher/eslint-plugin-import/pull/599

[@hulkish]: https://github.com/hulkish
[@timkraut]: https://github.com/timkraut
Expand All @@ -81,3 +83,4 @@ Yanked due to critical issue with cache key resulting from #839.
[@brettz9]: https://github.com/brettz9
[@JounQin]: https://github.com/JounQin
[@arcanis]: https://github.com/arcanis
[@sompylasar]: https://github.com/sompylasar
22 changes: 18 additions & 4 deletions utils/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const hashObject = require('./hash').hashObject
const CASE_SENSITIVE_FS = !fs.existsSync(path.join(__dirname, 'reSOLVE.js'))
exports.CASE_SENSITIVE_FS = CASE_SENSITIVE_FS

const ERROR_NAME = 'EslintPluginImportResolveError'

const fileExistsCache = new ModuleCache()

// Polyfill Node's `Module.createRequireFromPath` if not present (added in Node v10.12.0)
Expand Down Expand Up @@ -162,7 +164,9 @@ function resolverReducer(resolvers, map) {
return map
}

throw new Error('invalid resolver config')
const err = new Error('invalid resolver config')
err.name = ERROR_NAME
throw err
}

function getBaseDir(sourceFile) {
Expand All @@ -175,10 +179,14 @@ function requireResolver(name, sourceFile) {
tryRequire(path.resolve(getBaseDir(sourceFile), name))

if (!resolver) {
throw new Error(`unable to load resolver "${name}".`)
const err = new Error(`unable to load resolver "${name}".`)
err.name = ERROR_NAME
throw err
}
if (!isResolverValid(resolver)) {
throw new Error(`${name} with invalid interface loaded as resolver`)
const err = new Error(`${name} with invalid interface loaded as resolver`)
err.name = ERROR_NAME
throw err
}

return resolver
Expand Down Expand Up @@ -210,8 +218,14 @@ function resolve(p, context) {
)
} catch (err) {
if (!erroredContexts.has(context)) {
// The `err.stack` string starts with `err.name` followed by colon and `err.message`.
// We're filtering out the default `err.name` because it adds little value to the message.
let errMessage = err.message
if (err.name !== ERROR_NAME && err.stack) {
errMessage = err.stack.replace(/^Error: /, '')
}
context.report({
message: `Resolve error: ${err.message}`,
message: `Resolve error: ${errMessage}`,
loc: { line: 1, column: 0 },
})
erroredContexts.add(context)
Expand Down