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

[Fix] importType: avoid crashing on a non-string #2305

Merged

Conversation

lencioni
Copy link
Contributor

While writing imports in VSCode recently, I started to get errors throwm
from eslint that appear to originate from eslint-plugin-import.

Here's the strack trace from the output panel:

[Error - 3:52:43 PM] TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at validateString (internal/validators.js:124:11)
    at isAbsolute (path.js:1029:5)
    at isAbsolute (/path/to/repo/node_modules/eslint-plugin-import/src/core/importType.js:17:10)
    at typeTest (/path/to/repo/node_modules/eslint-plugin-import/src/core/importType.js:92:7)
    at resolveImportType (/path/to/repo/node_modules/eslint-plugin-import/src/core/importType.js:105:10)
    at reportIfMissing (/path/to/repo/node_modules/eslint-plugin-import/src/rules/no-extraneous-dependencies.js:170:7)
    at commonjs (/path/to/repo/node_modules/eslint-plugin-import/src/rules/no-extraneous-dependencies.js:267:7)
    at checkSourceValue (/path/to/repo/node_modules/eslint-module-utils/moduleVisitor.js:29:5)
    at checkSource (/path/to/repo/node_modules/eslint-module-utils/moduleVisitor.js:34:5)
    at /path/to/repo/node_modules/eslint/lib/linter/safe-emitter.js:45:58

I am able to trigger this consistently when writing an import, before I
start the open quote, like this:

import foo from

I found #1931
which pointed at a problem in the resolvers causing this problem. We do
use some custom resolvers with this plugin, so I tried disabling that
but the problem persisted.

Thankfully, I was able to reproduce this error in the test suite.

It would be good if this plugin handled this scenario gracefully instead
of throwing an error.

@ljharb
Copy link
Member

ljharb commented Nov 17, 2021

I don't think it's possible to have a test here, since this is invalid syntax, so eslint can't parse it. That said, I can repurpose this into a bug fix given that stack trace, thanks!

@ljharb ljharb force-pushed the no-extraneous-dependencies-failing-test branch from 2f91c2e to e8794f1 Compare November 17, 2021 23:13
@ljharb ljharb changed the title Add failing test for no-extraneous-dependencies [Fix] importType: avoid crashing on a non-string Nov 17, 2021
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #2305 (e8794f1) into main (7c239fe) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2305   +/-   ##
=======================================
  Coverage   94.64%   94.64%           
=======================================
  Files          65       65           
  Lines        2691     2691           
  Branches      891      891           
=======================================
  Hits         2547     2547           
  Misses        144      144           
Impacted Files Coverage Δ
src/core/importType.js 100.00% <ø> (ø)
src/rules/no-absolute-path.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c239fe...e8794f1. Read the comment docs.

@ljharb ljharb closed this Nov 18, 2021
@ljharb ljharb reopened this Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants