Skip to content

Js module search depth #6928

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

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
@@ -299,6 +299,11 @@ namespace ts {
name: "noImplicitUseStrict",
type: "boolean",
description: Diagnostics.Do_not_emit_use_strict_directives_in_module_output
},
{
name: "maxNodeSearchJsDepth",
type: "number",
description: Diagnostics.The_maximum_depth_of_JavaScript_modules_to_load_by_searching_node_modules
}
];

6 changes: 5 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
@@ -2171,7 +2171,7 @@
"category": "Error",
"code": 5059
},



"Concatenate and emit output to single file.": {
"category": "Message",
@@ -2450,6 +2450,10 @@
"category": "Message",
"code": 6112
},
"The maximum depth of JavaScript modules to load by searching node_modules": {
"category": "Message",
"code": 6085
},

"Variable '{0}' implicitly has an '{1}' type.": {
"category": "Error",
74 changes: 52 additions & 22 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
@@ -65,7 +65,7 @@ namespace ts {
: { resolvedModule: undefined, failedLookupLocations };
}
else {
return loadModuleFromNodeModules(moduleName, containingDirectory, host);
return loadModuleFromNodeModules(moduleName, containingDirectory, host, compilerOptions);
}
}

@@ -77,7 +77,7 @@ namespace ts {

/**
* @param {boolean} onlyRecordFailures - if true then function won't try to actually load files but instead record all attempts as failures. This flag is necessary
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations.
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations.
*/
function loadNodeModuleFromFile(extensions: string[], candidate: string, failedLookupLocation: string[], onlyRecordFailures: boolean, host: ModuleResolutionHost): string {
return forEach(extensions, tryLoad);
@@ -99,19 +99,27 @@ namespace ts {
const directoryExists = !onlyRecordFailures && directoryProbablyExists(candidate, host);
if (directoryExists && host.fileExists(packageJsonPath)) {

let jsonContent: { typings?: string };
let jsonContent: { typings?: string; main?: string };

try {
const jsonText = host.readFile(packageJsonPath);
jsonContent = jsonText ? <{ typings?: string }>JSON.parse(jsonText) : { typings: undefined };
jsonContent = jsonText ? <{ typings?: string; main?: string }>JSON.parse(jsonText) : { typings: undefined };
}
catch (e) {
// gracefully handle if readFile fails or returns not JSON
// gracefully handle if readFile fails or returns not JSON
jsonContent = { typings: undefined };
}

if (typeof jsonContent.typings === "string") {
const path = normalizePath(combinePaths(candidate, jsonContent.typings));
const result = loadNodeModuleFromFile(/*don't add extension*/[""], path, failedLookupLocation, !directoryProbablyExists(getDirectoryPath(path), host), host);
if (result) {
return result;
}
}
if (typeof jsonContent.main === "string") {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if ? do you want it to fall back to js and result in an error of unsupported file extension or just can not resolve module? i think we should not pick the js file if there is a .ts one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I get your comment or the else if prefix. Are you saying only try to load the file referenced by main if there was no typings setting?

I do notice one change I should make on review though: I should still use the supportedExtensions on the base path (i.e. if main points to file1.js, still look for file1.d.ts and file1.ts first (and also don't look for and load .js if not in the supportedExtensions list)). I'll update for that. Please clarify if I missed your original point.

// If 'main' points to 'foo.js', we still want to try and load 'foo.d.ts' and 'foo.ts' first (and only 'foo.js' if 'allowJs' is set).
const path = normalizePath(combinePaths(candidate, removeFileExtension(jsonContent.main)));
const result = loadNodeModuleFromFile(extensions, path, failedLookupLocation, !directoryProbablyExists(getDirectoryPath(path), host), host);
if (result) {
return result;
@@ -126,7 +134,7 @@ namespace ts {
return loadNodeModuleFromFile(extensions, combinePaths(candidate, "index"), failedLookupLocation, !directoryExists, host);
}

function loadModuleFromNodeModules(moduleName: string, directory: string, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations {
function loadModuleFromNodeModules(moduleName: string, directory: string, host: ModuleResolutionHost, compilerOptions: CompilerOptions): ResolvedModuleWithFailedLookupLocations {
const failedLookupLocations: string[] = [];
directory = normalizeSlashes(directory);
while (true) {
@@ -135,13 +143,14 @@ namespace ts {
const nodeModulesFolder = combinePaths(directory, "node_modules");
const nodeModulesFolderExists = directoryProbablyExists(nodeModulesFolder, host);
const candidate = normalizePath(combinePaths(nodeModulesFolder, moduleName));
// Load only typescript files irrespective of allowJs option if loading from node modules
let result = loadNodeModuleFromFile(supportedTypeScriptExtensions, candidate, failedLookupLocations, !nodeModulesFolderExists, host);

const supportedExtensions = getSupportedExtensions(compilerOptions);
let result = loadNodeModuleFromFile(supportedExtensions, candidate, failedLookupLocations, !nodeModulesFolderExists, host);
if (result) {
return { resolvedModule: { resolvedFileName: result, isExternalLibraryImport: true }, failedLookupLocations };
}

result = loadNodeModuleFromDirectory(supportedTypeScriptExtensions, candidate, failedLookupLocations, !nodeModulesFolderExists, host);
result = loadNodeModuleFromDirectory(supportedExtensions, candidate, failedLookupLocations, !nodeModulesFolderExists, host);
if (result) {
return { resolvedModule: { resolvedFileName: result, isExternalLibraryImport: true }, failedLookupLocations };
}
@@ -181,7 +190,7 @@ namespace ts {
searchName = normalizePath(combinePaths(searchPath, moduleName));
referencedSourceFile = forEach(supportedExtensions, extension => {
if (extension === ".tsx" && !compilerOptions.jsx) {
// resolve .tsx files only if jsx support is enabled
// resolve .tsx files only if jsx support is enabled
// 'logical not' handles both undefined and None cases
return undefined;
}
@@ -382,7 +391,7 @@ namespace ts {

const filesByName = createFileMap<SourceFile>();
// stores 'filename -> file association' ignoring case
// used to track cases when two file names differ only in casing
// used to track cases when two file names differ only in casing
const filesByNameIgnoreCase = host.useCaseSensitiveFileNames() ? createFileMap<SourceFile>(fileName => fileName.toLowerCase()) : undefined;

if (oldProgram) {
@@ -960,7 +969,7 @@ namespace ts {
}

// TypeScript 1.0 spec (April 2014): 12.1.6
// An ExternalImportDeclaration in an AmbientExternalModuleDeclaration may reference other external modules
// An ExternalImportDeclaration in an AmbientExternalModuleDeclaration may reference other external modules
// only through top - level external module names. Relative external module names are not permitted.
if (!inAmbientModule || !isExternalModuleNameRelative((<LiteralExpression>moduleNameExpr).text)) {
(imports || (imports = [])).push(<LiteralExpression>moduleNameExpr);
@@ -978,7 +987,7 @@ namespace ts {
(moduleAugmentations || (moduleAugmentations = [])).push(moduleName);
}
else if (!inAmbientModule) {
// An AmbientExternalModuleDeclaration declares an external module.
// An AmbientExternalModuleDeclaration declares an external module.
// This type of declaration is permitted only in the global module.
// The StringLiteral must specify a top - level external module name.
// Relative external module names are not permitted
@@ -1055,7 +1064,7 @@ namespace ts {
}

// Get source file from normalized fileName
function findSourceFile(fileName: string, path: Path, isDefaultLib: boolean, refFile?: SourceFile, refPos?: number, refEnd?: number): SourceFile {
function findSourceFile(fileName: string, path: Path, isDefaultLib: boolean, refFile?: SourceFile, refPos?: number, refEnd?: number, isFileFromNodeSearch?: boolean): SourceFile {
if (filesByName.contains(path)) {
const file = filesByName.get(path);
// try to check if we've already seen this file but with a different casing in path
@@ -1064,6 +1073,13 @@ namespace ts {
reportFileNamesDifferOnlyInCasingError(fileName, file.fileName, refFile, refPos, refEnd);
}

// If this was a file found by a node_modules search, set the nodeModuleSearchDistance to parent distance + 1.
if (isFileFromNodeSearch) {
const newDistance = (refFile && refFile.nodeModuleSearchDistance) === undefined ? 1 : refFile.nodeModuleSearchDistance + 1;
// If already set on the file, don't overwrite if it was already found closer (which may be '0' if added as a root file)
file.nodeModuleSearchDistance = (typeof file.nodeModuleSearchDistance === "number") ? Math.min(file.nodeModuleSearchDistance, newDistance) : newDistance;
}

return file;
}

@@ -1082,6 +1098,12 @@ namespace ts {
if (file) {
file.path = path;

// Default to same distance as parent. Add one if found by a search.
file.nodeModuleSearchDistance = (refFile && refFile.nodeModuleSearchDistance) || 0;
if (isFileFromNodeSearch) {
file.nodeModuleSearchDistance++;
}

if (host.useCaseSensitiveFileNames()) {
// for case-sensitive file systems check if we've already seen some file with similar filename ignoring case
const existingFile = filesByNameIgnoreCase.get(path);
@@ -1126,28 +1148,36 @@ namespace ts {
}

function processImportedModules(file: SourceFile, basePath: string) {
const maxJsNodeModuleSearchDistance = options.maxNodeSearchJsDepth || 0;
collectExternalModuleReferences(file);
if (file.imports.length || file.moduleAugmentations.length) {
file.resolvedModules = {};
const moduleNames = map(concatenate(file.imports, file.moduleAugmentations), getTextOfLiteral);
const resolutions = resolveModuleNamesWorker(moduleNames, getNormalizedAbsolutePath(file.fileName, currentDirectory));
file.nodeModuleSearchDistance = file.nodeModuleSearchDistance || 0;
for (let i = 0; i < moduleNames.length; i++) {
const resolution = resolutions[i];
setResolvedModule(file, moduleNames[i], resolution);
// add file to program only if:
// - resolution was successfull
// - noResolve is falsy
// - module name come from the list fo imports
const shouldAddFile = resolution &&
!options.noResolve &&
i < file.imports.length;
// - it's not a top level JavaScript module that exceeded the search max
const exceedsJsSearchDepth = resolution && resolution.isExternalLibraryImport &&
hasJavaScriptFileExtension(resolution.resolvedFileName) &&
file.nodeModuleSearchDistance >= maxJsNodeModuleSearchDistance;
const shouldAddFile = resolution && !options.noResolve && i < file.imports.length && !exceedsJsSearchDepth;

if (shouldAddFile) {
const importedFile = findSourceFile(resolution.resolvedFileName, toPath(resolution.resolvedFileName, currentDirectory, getCanonicalFileName), /*isDefaultLib*/ false, file, skipTrivia(file.text, file.imports[i].pos), file.imports[i].end);

if (importedFile && resolution.isExternalLibraryImport) {
// Since currently irrespective of allowJs, we only look for supportedTypeScript extension external module files,
// this check is ok. Otherwise this would be never true for javascript file
const importedFile = findSourceFile(resolution.resolvedFileName,
toPath(resolution.resolvedFileName, currentDirectory, getCanonicalFileName),
/*isDefaultLib*/ false,
file,
skipTrivia(file.text, file.imports[i].pos),
file.imports[i].end,
resolution.isExternalLibraryImport);

if (importedFile && resolution.isExternalLibraryImport && fileExtensionIs(importedFile.fileName, ".ts")) {
if (!isExternalModule(importedFile) && importedFile.statements.length) {
const start = getTokenPosOfNode(file.imports[i], file);
fileProcessingDiagnostics.add(createFileDiagnostic(file, start, file.imports[i].end - start, Diagnostics.Exported_external_package_typings_file_0_is_not_a_module_Please_contact_the_package_author_to_update_the_package_definition, importedFile.fileName));
3 changes: 3 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
@@ -1565,6 +1565,8 @@ namespace ts {
/* @internal */ externalModuleIndicator: Node;
// The first node that causes this file to be a CommonJS module
/* @internal */ commonJsModuleIndicator: Node;
// The number of times node_modules was searched to locate the package containing this file
/* @internal */ nodeModuleSearchDistance?: number;

/* @internal */ identifiers: Map<string>;
/* @internal */ nodeCount: number;
@@ -2436,6 +2438,7 @@ namespace ts {
allowSyntheticDefaultImports?: boolean;
allowJs?: boolean;
noImplicitUseStrict?: boolean;
maxNodeSearchJsDepth?: number;
/* @internal */ stripInternal?: boolean;

// Skip checking lib.d.ts to help speed up tests.
3 changes: 2 additions & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
@@ -2033,7 +2033,7 @@ namespace ts {
else {
const sourceFiles = targetSourceFile === undefined ? host.getSourceFiles() : [targetSourceFile];
for (const sourceFile of sourceFiles) {
if (!isDeclarationFile(sourceFile)) {
if (!isDeclarationFile(sourceFile) && !sourceFile.nodeModuleSearchDistance) {
onSingleFileEmit(host, sourceFile);
}
}
@@ -2068,6 +2068,7 @@ namespace ts {
// Can emit only sources that are not declaration file and are either non module code or module with --module or --target es6 specified
const bundledSources = filter(host.getSourceFiles(),
sourceFile => !isDeclarationFile(sourceFile) && // Not a declaration file
!sourceFile.nodeModuleSearchDistance && // Not loaded from searching under node_modules
(!isExternalModule(sourceFile) || // non module file
(getEmitModuleKind(options) && isExternalModule(sourceFile)))); // module that can emit - note falsy value from getEmitModuleKind means the module kind that shouldn't be emitted
if (bundledSources.length) {
2 changes: 0 additions & 2 deletions tests/baselines/reference/nodeResolution6.js
Original file line number Diff line number Diff line change
@@ -13,7 +13,5 @@ export declare var y;
import y = require("a");


//// [ref.js]
var x = 1;
//// [b.js]
"use strict";
2 changes: 0 additions & 2 deletions tests/baselines/reference/nodeResolution8.js
Original file line number Diff line number Diff line change
@@ -12,7 +12,5 @@ export declare var y;
//// [b.ts]
import y = require("a");

//// [ref.js]
var x = 1;
//// [b.js]
"use strict";
3 changes: 3 additions & 0 deletions tests/cases/unittests/moduleResolution.ts
Original file line number Diff line number Diff line change
@@ -179,6 +179,9 @@ module ts {
"/a/b/foo.ts",
"/a/b/foo.tsx",
"/a/b/foo.d.ts",
"/c/d.ts",
"/c/d.tsx",
"/c/d.d.ts",
"/a/b/foo/index.ts",
"/a/b/foo/index.tsx",
]);