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

Conversation

sompylasar
Copy link
Contributor

Resolve errors are most likely caused by invalid configuration, and the reason is easier to determine with the full details rather than just err.message.

See #536

With this change, it reports something like:

import/no-unresolved: Resolve error: SyntaxError: Unexpected token import
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at module.exports (/__censored__/webpack/configFactory.js:216:3)
    at configProdClient (/__censored__/webpack/configProdClient.js:5:36)
    at Object.<anonymous> (/__censored__/webpack/configForEslintImportResolver.js:1:126)

Fixes import-js#536.

Resolve errors are most likely caused by invalid configuration,
and the reason is easier to determine with the full details
rather than just `err.message`.

With this change, it reports something like:
```
import/no-unresolved: Resolve error: SyntaxError: Unexpected token import
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at module.exports (/__censored__/webpack/configFactory.js:216:3)
    at configProdClient (/__censored__/webpack/configProdClient.js:5:36)
    at Object.<anonymous> (/__censored__/webpack/configForEslintImportResolver.js:1:126)
```
@benmosher
Copy link
Member

Ah, cool. Makes sense, though in retrospect I'm thinking maybe the stacktrace should be emitted by a debug logger instead of the error message, since it's so verbose.

What do you think?

@sompylasar
Copy link
Contributor Author

sompylasar commented Oct 23, 2016

I think the stack for this specific error is meaningful at the exact place where the error has occurred, to be able to quickly fix the error without switching context.

I use Atom Linter, so I expect all the error details in the linter error message which editor shows me.

We could also log the error, but the core benefit is being able to see its cause quickly.

@ghost
Copy link

ghost commented Oct 16, 2018

Is this PR moving? It would really be helpful.

@sompylasar
Copy link
Contributor Author

@st-sloth Looks like the tests need to be updated to reflect the change. I'll have a look if time allows.

@sompylasar
Copy link
Contributor Author

Changed the logic slightly to only add stack to unknown errors (that do not originate from eslint-plugin-import itself, which is designated by the error name of 'EslintPluginImportResolveError'). Updated the tests to reflect the change.

@coveralls
Copy link

coveralls commented Oct 29, 2018

Coverage Status

Coverage increased (+0.03%) to 96.343% when pulling aed7a73 on sompylasar:patch-1 into b511da2 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.356% when pulling 931363f on sompylasar:patch-1 into db471a8 on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.356% when pulling 931363f on sompylasar:patch-1 into db471a8 on benmosher:master.

@ljharb ljharb added the package: utils eslint-module-utils package label Jan 5, 2020
@ljharb ljharb changed the title Fix #536: For Resolve error report err.stack utils: [Fix] report the error stack on a resolution error Jan 5, 2020
@ljharb ljharb merged commit aed7a73 into import-js:master Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: utils eslint-module-utils package
Development

Successfully merging this pull request may close these issues.

4 participants