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

sompylasar commented Oct 2, 2016

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 #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

This comment has been minimized.

Copy link
Owner

benmosher commented Oct 23, 2016

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

This comment has been minimized.

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.

@st-sloth

This comment has been minimized.

Copy link
Contributor

st-sloth commented Oct 16, 2018

Is this PR moving? It would really be helpful.

@sompylasar

This comment has been minimized.

Copy link
Contributor Author

sompylasar commented Oct 16, 2018

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

@sompylasar sompylasar force-pushed the sompylasar:patch-1 branch from b4f7e7e to 931363f Oct 29, 2018
@sompylasar

This comment has been minimized.

Copy link
Contributor Author

sompylasar commented Oct 29, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

coveralls commented Oct 29, 2018

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

This comment has been minimized.

Copy link

coveralls commented Oct 29, 2018

Coverage Status

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

@ljharb ljharb force-pushed the sompylasar:patch-1 branch from 931363f to aed7a73 Jan 5, 2020
@ljharb
ljharb approved these changes 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 benmosher:master Jan 5, 2020
1 of 5 checks passed
1 of 5 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage decreased (-0.04%) to 96.274%
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.