[New] no extraneous type-only dependencies #2234

jablko · 2021-09-21T20:32:12Z

If eslint-import-resolver-typescript is installed, instead of ignoring type-only imports entirely, it would be handy if no-extraneous-dependencies reported undeclared e.g. @types dependencies? In other words, resolve type-only imports with eslint-resolver-typescript or not at all, which is what no-extraneous-dependencies does right now.

eslint-import-resolve-typescript is required because other resolvers can't resolve types, so can't tell whether they resolve to e.g. json-schema or @types/json-schema and so can't tell if the dependency is declared or missing.

TypeScript compilation confirms that types are resolved, but it doesn't confirm that those dependencies are declared in the package.json, so no-extraneous-dependencies can be beneficial.

jablko · 2021-09-21T21:59:43Z

package.json

@@ -71,7 +71,7 @@
    "cross-env": "^4.0.0",
    "eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0",
    "eslint-import-resolver-node": "file:./resolvers/node",
-    "eslint-import-resolver-typescript": "^1.0.2 || ^1.1.1",
+    "eslint-import-resolver-typescript": "^1.0.2 || ^2.5.0",


eslint-import-resolver-typescript handles @types packages since version 2.0.0.

i'd prefer to continue testing on v1.1.1

Suggested change

"eslint-import-resolver-typescript": "^1.0.2 || ^2.5.0",

"eslint-import-resolver-typescript": "^1.0.2 || ^1.1.1 || ^2.5.0",

jablko · 2021-09-21T21:59:46Z

src/rules/no-extraneous-dependencies.js

@@ -117,6 +117,28 @@ function optDepErrorMessage(packageName) {
    `not optionalDependencies.`;
 }

+function resolveTypeOnly(modulePath, context) {
+  const sourceFile = context.getPhysicalFilename ? context.getPhysicalFilename() : context.getFilename();
+  const configResolvers = context.settings['import/resolver'];


Copied from:

eslint-plugin-import/utils/resolve.js

Lines 130 to 139 in 4f0f560

const configResolvers = (settings['import/resolver']

|| { 'node': settings['import/resolve'] }); // backward compatibility

const resolvers = resolverReducer(configResolvers, new Map());

for (const pair of resolvers) {

const name = pair[0];

const config = pair[1];

const resolver = requireResolver(name, sourceFile);

const resolved = withResolver(resolver, config);

jablko · 2021-09-21T21:59:52Z

src/rules/no-extraneous-dependencies.js

@@ -196,7 +219,7 @@ function reportIfMissing(context, deps, depsOptions, node, name) {

    if (
      declarationStatus.isInDeps ||
-      (depsOptions.allowDevDeps && declarationStatus.isInDevDeps) ||
+      ((depsOptions.allowDevDeps || isTypeOnly) && declarationStatus.isInDevDeps) ||


It's common practice to declare type-only dependencies in devDependencies -- that was among the reasons for originally ignoring type-only imports: #1618

codecov · 2021-09-22T04:00:51Z

Codecov Report

Merging #2234 (9403ffa) into main (46c4709) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2234      +/-   ##
==========================================
+ Coverage   94.60%   94.63%   +0.02%     
==========================================
  Files          65       65              
  Lines        2688     2702      +14     
  Branches      888      896       +8     
==========================================
+ Hits         2543     2557      +14     
  Misses        145      145

Impacted Files	Coverage Δ
src/rules/no-extraneous-dependencies.js	`100.00% <100.00%> (ø)`
utils/resolve.js	`88.13% <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 46c4709...9403ffa. Read the comment docs.

ljharb · 2021-09-22T05:13:49Z

package.json

@@ -71,7 +71,7 @@
    "cross-env": "^4.0.0",
    "eslint": "^2 || ^3 || ^4 || ^5 || ^6 || ^7.2.0",
    "eslint-import-resolver-node": "file:./resolvers/node",
-    "eslint-import-resolver-typescript": "^1.0.2 || ^1.1.1",
+    "eslint-import-resolver-typescript": "^1.0.2 || ^2.5.0",


i'd prefer to continue testing on v1.1.1

Suggested change

"eslint-import-resolver-typescript": "^1.0.2 || ^2.5.0",

"eslint-import-resolver-typescript": "^1.0.2 || ^1.1.1 || ^2.5.0",

ljharb · 2021-09-22T05:14:47Z

tests/src/rules/no-extraneous-dependencies.js

@@ -378,7 +380,7 @@ ruleTester.run('no-extraneous-dependencies', rule, {
  ],
 });

-describe('TypeScript', () => {
+(semver.satisfies(typescriptResolverPkg.version, '>=2.1.0') ? describe : describe.skip)('TypeScript', () => {


this means the files in here won't be tested on v1 (v1.0 or v1.1) of the TS resolver. can we preserve that?

Yes but ... This only works with >=2.0.0 of the resolver because earlier versions didn't look in @types packages, so testing against them doesn't confirm any special type-only treatment because they return the same thing as the node resolver? Like we exclude typescript-eslint-parser because it predates type-only imports.

I can see adding a "Legacy TypeScript resolver" test and running that in case of an earlier version. I think I'd prefer not, and keep the tests focused on the desired behavior (like we exclude typescript-eslint-parser). Maybe there's a reason it's important though? I'm happy either way!

Mainly semver - things that work should keep working, and the tests ensure that's the case.

Fair enough. I've now added a test to confirm that the behavior of earlier versions is unchanged?

utils/resolve.js

erlichmen · 2021-11-17T09:28:45Z

is this PR still relevant? with the latest (eslint v8, eslint-import-resolver-typescript v2.5. and eslint-plugin-import: v2.25.3) I'm still getting "ESLint: '@types/XXXX' should be listed in the project's dependencies, not devDependencies. (import/no-extraneous-dependencies).

ljharb · 2023-01-11T23:06:06Z

@jablko it'd be great to get this rebased :-)

jablko force-pushed the patch-4 branch 6 times, most recently from eedd9f1 to 8ef6706 Compare September 21, 2021 21:38

jablko commented Sep 21, 2021

View reviewed changes

jablko marked this pull request as ready for review September 21, 2021 22:01

ljharb reviewed Sep 22, 2021

View reviewed changes

jablko force-pushed the patch-4 branch 4 times, most recently from 8cc855e to 6faf7d8 Compare September 22, 2021 23:02

jablko added 3 commits October 25, 2021 12:43

[New] no extraneous type-only dependencies

895f49b

Add tests

655a01f

Test legacy TypeScript resolver

9403ffa

jablko force-pushed the patch-4 branch from 6faf7d8 to 9403ffa Compare October 25, 2021 19:46

ljharb marked this pull request as draft January 11, 2023 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] no extraneous type-only dependencies #2234

[New] no extraneous type-only dependencies #2234

jablko commented Sep 21, 2021

jablko Sep 21, 2021

ljharb Sep 22, 2021

jablko Sep 21, 2021

jablko Sep 21, 2021

codecov bot commented Sep 22, 2021 •

edited

Loading

ljharb Sep 22, 2021

ljharb Sep 22, 2021

jablko Sep 22, 2021

ljharb Sep 22, 2021

jablko Sep 22, 2021

erlichmen commented Nov 17, 2021

ljharb commented Jan 11, 2023

	"eslint-import-resolver-typescript": "^1.0.2 \|\| ^2.5.0",
	"eslint-import-resolver-typescript": "^1.0.2 \|\| ^1.1.1 \|\| ^2.5.0",

	const configResolvers = (settings['import/resolver']
	\|\| { 'node': settings['import/resolve'] }); // backward compatibility

	const resolvers = resolverReducer(configResolvers, new Map());

	for (const pair of resolvers) {
	const name = pair[0];
	const config = pair[1];
	const resolver = requireResolver(name, sourceFile);
	const resolved = withResolver(resolver, config);

[New] no extraneous type-only dependencies #2234

Are you sure you want to change the base?

[New] no extraneous type-only dependencies #2234

Conversation

jablko commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlichmen commented Nov 17, 2021

ljharb commented Jan 11, 2023

codecov bot commented Sep 22, 2021 •

edited

Loading