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

[New] no extraneous type-only dependencies #2234

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Sep 21, 2021

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 jablko force-pushed the patch-4 branch 6 times, most recently from eedd9f1 to 8ef6706 Compare September 21, 2021 21:38
@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

@@ -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'];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from:

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);

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

if (
declarationStatus.isInDeps ||
(depsOptions.allowDevDeps && declarationStatus.isInDevDeps) ||
((depsOptions.allowDevDeps || isTypeOnly) && declarationStatus.isInDevDeps) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

codecov bot commented Sep 22, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

@@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

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

describe('TypeScript', () => {
(semver.satisfies(typescriptResolverPkg.version, '>=2.1.0') ? describe : describe.skip)('TypeScript', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

utils/resolve.js Outdated Show resolved Hide resolved
@jablko jablko force-pushed the patch-4 branch 4 times, most recently from 8cc855e to 6faf7d8 Compare September 22, 2021 23:02
@erlichmen
Copy link

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
Copy link
Member

ljharb commented Jan 11, 2023

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

@ljharb ljharb marked this pull request as draft January 11, 2023 23:06
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.

3 participants