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.

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

Respect importModuleSpecifierEnding inside node_modules packages #48995

Merged
merged 2 commits into from
May 6, 2022
Merged

Respect importModuleSpecifierEnding inside node_modules packages #48995

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
736c8a1
Respect importModuleSpecifierEnding inside node_modules packages
andrewbranch May 6, 2022
bf6b47f
Add tests for missing package.json
andrewbranch May 6, 2022
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
47 changes: 24 additions & 23 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ namespace ts.moduleSpecifiers {
const info = getInfo(importingSourceFile.path, host);
const modulePaths = getAllModulePaths(importingSourceFile.path, nodeModulesFileName, host, preferences, options);
return firstDefined(modulePaths,
modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, /*packageNameOnly*/ true, options.overrideImportMode));
modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, preferences, /*packageNameOnly*/ true, options.overrideImportMode));
}

function getModuleSpecifierWorker(
Expand All @@ -120,7 +120,7 @@ namespace ts.moduleSpecifiers {
): string {
const info = getInfo(importingSourceFileName, host);
const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host, userPreferences, options);
return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, /*packageNameOnly*/ undefined, options.overrideImportMode)) ||
return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode)) ||
getLocalModuleSpecifier(toFileName, info, compilerOptions, host, preferences);
}

Expand Down Expand Up @@ -248,7 +248,7 @@ namespace ts.moduleSpecifiers {
let pathsSpecifiers: string[] | undefined;
let relativeSpecifiers: string[] | undefined;
for (const modulePath of modulePaths) {
const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, /*packageNameOnly*/ undefined, options.overrideImportMode);
const specifier = tryGetModuleNameAsNodeModule(modulePath, info, importingSourceFile, host, compilerOptions, userPreferences, /*packageNameOnly*/ undefined, options.overrideImportMode);
nodeModulesSpecifiers = append(nodeModulesSpecifiers, specifier);
if (specifier && modulePath.isRedirect) {
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
Expand Down Expand Up @@ -666,7 +666,7 @@ namespace ts.moduleSpecifiers {
: removeFileExtension(relativePath);
}

function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, importingSourceFile: SourceFile , host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean, overrideMode?: ModuleKind.ESNext | ModuleKind.CommonJS): string | undefined {
function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, importingSourceFile: SourceFile , host: ModuleSpecifierResolutionHost, options: CompilerOptions, userPreferences: UserPreferences, packageNameOnly?: boolean, overrideMode?: ModuleKind.ESNext | ModuleKind.CommonJS): string | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

this line is comically long

if (!host.fileExists || !host.readFile) {
return undefined;
}
Expand All @@ -680,8 +680,9 @@ namespace ts.moduleSpecifiers {
let moduleSpecifier = path;
let isPackageRootPath = false;
if (!packageNameOnly) {
const preferences = getPreferences(host, userPreferences, options, importingSourceFile);
let packageRootIndex = parts.packageRootIndex;
let moduleFileNameForExtensionless: string | undefined;
let moduleFileName: string | undefined;
while (true) {
// If the module could be imported by a directory name, use that directory's name
const { moduleFileToTry, packageRootPath, blockedByExports, verbatimFromExports } = tryDirectoryWithPackageJson(packageRootIndex);
Expand All @@ -698,12 +699,12 @@ namespace ts.moduleSpecifiers {
isPackageRootPath = true;
break;
}
if (!moduleFileNameForExtensionless) moduleFileNameForExtensionless = moduleFileToTry;
if (!moduleFileName) moduleFileName = moduleFileToTry;

// try with next level of directory
packageRootIndex = path.indexOf(directorySeparator, packageRootIndex + 1);
if (packageRootIndex === -1) {
moduleSpecifier = getExtensionlessFileName(moduleFileNameForExtensionless);
moduleSpecifier = removeExtensionAndIndexPostFix(moduleFileName, preferences.ending, options, host);
break;
}
}
Expand Down Expand Up @@ -768,28 +769,22 @@ namespace ts.moduleSpecifiers {
}
}
// If the file is the main module, it can be imported by the package name
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main;
const mainFileRelative = packageJsonContent.typings || packageJsonContent.types || packageJsonContent.main || "index.js";
if (isString(mainFileRelative)) {
const mainExportFile = toPath(mainFileRelative, packageRootPath, getCanonicalFileName);
if (removeFileExtension(mainExportFile) === removeFileExtension(getCanonicalFileName(moduleFileToTry))) {
return { packageRootPath, moduleFileToTry };
}
}
}
return { moduleFileToTry };
}

function getExtensionlessFileName(path: string): string {
andrewbranch marked this conversation as resolved.
Show resolved Hide resolved
// We still have a file name - remove the extension
const fullModulePathWithoutExtension = removeFileExtension(path);

// If the file is /index, it can be imported by its directory name
// IFF there is not _also_ a file by the same name
if (getCanonicalFileName(fullModulePathWithoutExtension.substring(parts.fileNameIndex)) === "/index" && !tryGetAnyFileFromPath(host, fullModulePathWithoutExtension.substring(0, parts.fileNameIndex))) {
return fullModulePathWithoutExtension.substring(0, parts.fileNameIndex);
else {
// No package.json exists; an index.js will still resolve as the package name
const fileName = getCanonicalFileName(moduleFileToTry.substring(parts.packageRootIndex + 1));
if (fileName === "index.d.ts" || fileName === "index.js" || fileName === "index.ts" || fileName === "index.tsx") {
return { moduleFileToTry, packageRootPath };
}
}

return fullModulePathWithoutExtension;
return { moduleFileToTry };
}
}

Expand All @@ -812,13 +807,19 @@ namespace ts.moduleSpecifiers {
});
}

function removeExtensionAndIndexPostFix(fileName: string, ending: Ending, options: CompilerOptions): string {
function removeExtensionAndIndexPostFix(fileName: string, ending: Ending, options: CompilerOptions, host?: ModuleSpecifierResolutionHost): string {
if (fileExtensionIsOneOf(fileName, [Extension.Json, Extension.Mjs, Extension.Cjs])) return fileName;
const noExtension = removeFileExtension(fileName);
if (fileExtensionIsOneOf(fileName, [Extension.Dmts, Extension.Mts, Extension.Dcts, Extension.Cts])) return noExtension + getJSExtensionForFile(fileName, options);
switch (ending) {
case Ending.Minimal:
return removeSuffix(noExtension, "/index");
const withoutIndex = removeSuffix(noExtension, "/index");
if (host && withoutIndex !== noExtension && tryGetAnyFileFromPath(host, withoutIndex)) {
// Can't remove index if there's a file by the same name as the directory.
// Probably more callers should pass `host` so we can determine this?
return noExtension;
}
return withoutIndex;
case Ending.Index:
return noExtension;
case Ending.JsExtension:
Expand Down
17 changes: 17 additions & 0 deletions tests/cases/fourslash/importNameCodeFixJsEnding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts"/>

// @module: commonjs

// @Filename: /node_modules/lit/package.json
//// { "name": "lit", "version": "1.0.0" }

// @Filename: /node_modules/lit/index.d.ts
//// import "./decorators";

// @Filename: /node_modules/lit/decorators.d.ts
//// export declare function customElement(name: string): any;

// @Filename: /a.ts
//// customElement/**/

verify.importFixModuleSpecifiers("", ["lit/decorators.js"], { importModuleSpecifierEnding: "js" })
Copy link
Member

Choose a reason for hiding this comment

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

should there be a test for .cjs/.mjs as well, since you mentioned that they were broken before?

Copy link
Member

Choose a reason for hiding this comment

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

also, are there already tests for filename/package conflicts? I assume yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are .cjs tests. Filename/package conflicts aren’t related to this change, I think.