Skip to content

fix(type-utils): handle external packages that are not in program.sourceFileToPackageName #10097

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

fix(type-utils): handle external packages that are not in program.sourceFileToPackageName #10097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cddd246
restore old logic
abrahamguo Oct 3, 2024
615f0cf
lint and add additional test
abrahamguo Oct 3, 2024
25e5120
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into f…
abrahamguo Oct 11, 2024
99a21ff
WIP
abrahamguo Oct 12, 2024
89eadc9
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into f…
abrahamguo Oct 17, 2024
95f33ee
handle type roots
abrahamguo Oct 17, 2024
567760c
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into f…
abrahamguo Nov 10, 2024
62d2bcd
lint
abrahamguo Nov 10, 2024
c3730c4
check node_modules's of parent directories
abrahamguo Nov 10, 2024
3c2ffaa
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into f…
abrahamguo Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions packages/type-utils/src/typeOrValueSpecifiers/typeDeclaredInPackageDeclarationFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { getCanonicalFileName } from '@typescript-eslint/typescript-estree';
import path from 'node:path';
import * as ts from 'typescript';

function findParentModuleDeclaration(
Expand Down Expand Up @@ -30,15 +32,36 @@ function typeDeclaredInDeclarationFile(
declarationFiles: ts.SourceFile[],
program: ts.Program,
): boolean {
// Handle scoped packages: if the name starts with @, remove it and replace / with __
const typesPackageName = packageName.replace(/^@([^/]+)\//, '$1__');
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_expressions#escaping
const escapeAlternates = (alternates: string[]): string =>
alternates
.map(alternate => alternate.replaceAll(/[.*+?^${}()|[\]\\]/g, '\\$&'))
.join('|');

const matcher = new RegExp(`${packageName}|${typesPackageName}`);
const packageNameRegExp = escapeAlternates([
packageName,
// Handle scoped packages: if the name starts with @, remove it and replace / with __
`@types/${packageName.replace(/^@([^/]+)\//, '$1__')}`,
Copy link
Member

Choose a reason for hiding this comment

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

[Bug] @types/ should not be hardcoded to always be considered. From https://www.typescriptlang.org/tsconfig/#typeRoots:

If typeRoots is specified, only packages under typeRoots will be included.

]);
const packageNameMatcher = new RegExp(packageNameRegExp);
const { typeRoots } = program.getCompilerOptions();
const fileNameMatcher = new RegExp(
`(?:${escapeAlternates(
typeRoots
? typeRoots.map(typeRoot =>
getCanonicalFileName(
path.join(program.getCurrentDirectory(), typeRoot),
),
)
: ['node_modules'],
)})/(?:${packageNameRegExp})/`,
);
Copy link
Member

Choose a reason for hiding this comment

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

https://www.typescriptlang.org/tsconfig/#typeRoots - if someone customizes this, then fileNameMatcher will be wrong. This'll need to reflect the project's compilerOptions.typeRoots.

Copy link
Member

Choose a reason for hiding this comment

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

(moved to https://github.com/typescript-eslint/typescript-eslint/pull/10097/files#r1854146022 to be closer to implementation)

return declarationFiles.some(declaration => {
const packageIdName = program.sourceFileToPackageName.get(declaration.path);
return (
packageIdName !== undefined &&
matcher.test(packageIdName) &&
(packageIdName == null
? fileNameMatcher.test(getCanonicalFileName(declaration.fileName))
: packageNameMatcher.test(packageIdName)) &&
program.isSourceFileFromExternalLibrary(declaration)
);
});
Expand Down
13 changes: 12 additions & 1 deletion packages/type-utils/tests/TypeOrValueSpecifier.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { parseForESLint } from '@typescript-eslint/parser';
import Ajv from 'ajv';
import path from 'node:path';

import type { TypeOrValueSpecifier } from '../src/TypeOrValueSpecifier';
import type { TypeOrValueSpecifier } from '../src';

import { typeMatchesSpecifier, typeOrValueSpecifiersSchema } from '../src';

Expand Down Expand Up @@ -366,6 +366,17 @@ describe('TypeOrValueSpecifier', () => {
package: 'node:test',
},
],
[
`
import { Buffer } from 'node:buffer';
type Test = Buffer;
`,
{ from: 'package', name: 'Buffer', package: 'node' },
],
[
'type Test = Buffer;',
{ from: 'package', name: 'Buffer', package: 'node' },
],
])('matches a matching package specifier: %s', runTestPositive);

it.each<[string, TypeOrValueSpecifier]>([
Expand Down
Loading