Implement maxNodeModuleJsDepth, noResolve #1189

jakebailey · 2025-06-14T21:10:49Z

Fixes #931

This was tricky to deal with in a concurrent loader.

The gist of it is that we keep track of the current "JS file depth" as we recurse. If a subtask about to be enqueued is a JS file from node_modules, we skip it if the current depth of JS files is too high. If the depth is okay, we'll load it, then recurse. If we ever revisit the same file again, we check to see if the new depth is lower than the last time the file is checked, and if it is we'll recheck its subtasks at that new depth (potentially loading more files).

While here, I've simplified a few things; namely I remembered the trick to have a self-referencing generic method and implemented noResolve.

Sadly, there's now a mutex per file, but I can't think of a cleaner method. The only place where this would matter is when the same file is referenced multiple times, in which case we need to recheck it serially anyhow.

We don't seem to have any tests for maxNodeModuleJsDepth; I'll think about how to make some. I'm also not setting up maxNodeModuleJsDepth for the the editor. I'm not sure where that goes.

Copilot

Pull Request Overview

This PR introduces depth-based controls (maxNodeModuleJsDepth) for loading JS files in node_modules and implements the noResolve option to skip adding resolved modules to the program. It refactors task-loading to track whether a file is loaded and if its depth should increase, simplifying concurrent loading logic.

Add loaded, shouldIncreaseDepth, and isLoaded methods to parse- and project-reference tasks
Introduce a generic fileLoaderWorker that tracks per-file mutexes and depth limits
Update fileloader.go to use a resolvedRef type, respect maxNodeModuleJsDepth, and honor noResolve

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
testdata/baselines/reference/.../moduleResolutionNoResolve.js.diff	Removes outdated diff for `noResolve` baseline
testdata/baselines/reference/.../moduleResolutionNoResolve.js	Updates the baseline output for `noResolve`
internal/compiler/projectreferenceparsetask.go	Rename `start`→`load`, add depth/no-resolve stubs
internal/compiler/program.go	Remove unused depth-tracking fields in `Program`
internal/compiler/parsetask.go	Refactor subtasks to use `resolvedRef`, track JS depth
internal/compiler/fileloadertask.go	Generic worker with mutex per file, depth-first logic
internal/compiler/fileloader.go	Wire up `maxNodeModuleJsDepth`, implement `noResolve` logic

Comments suppressed due to low confidence (2)

internal/compiler/fileloader.go:67

There are no tests covering the new maxNodeModuleJsDepth behavior; consider adding cases where the loader skips JS files beyond the depth limit.

var maxNodeModuleJsDepth int

internal/compiler/fileloader.go:386

The new noResolve flag path isn't exercised by existing tests; add tests to verify that setting noResolve prevents modules from being added.

shouldAddFile := resolvedModule.IsResolved() && optionsForFile.NoResolve.IsFalseOrUnknown()

jakebailey · 2025-06-15T04:22:36Z

testdata/baselines/reference/submoduleAccepted/compiler/jsEnumTagOnObjectFrozen.errors.txt.diff

+index.js(10,12): error TS2749: 'Thing' refers to a value, but is being used as a type here. Did you mean 'typeof Thing'?
+index.js(17,16): error TS2749: 'Thing' refers to a value, but is being used as a type here. Did you mean 'typeof Thing'?


Expected, since @enum doesn't do anything anymore and so no type is declared.

jakebailey · 2025-06-15T04:24:06Z

...nes/reference/submoduleAccepted/conformance/checkExportsObjectAssignProperty.errors.txt.diff

+-
+-
+-==== validator.ts (10 errors) ====
+index.js(4,11): error TS2580: Cannot find name 'require'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.


mod1 looks like:

Object.defineProperty(exports, "thing", { value: 42, writable: true }); Object.defineProperty(exports, "readonlyProp", { value: "Smith", writable: false }); Object.defineProperty(exports, "rwAccessors", { get() { return 98122 }, set(_) { /*ignore*/ } }); Object.defineProperty(exports, "readonlyAccessor", { get() { return 21.75 } }); Object.defineProperty(exports, "setonlyAccessor", { /** @param {string} str */ set(str) { this.rwAccessors = Number(str) } });

This is not being treated as a module for some reason? I don't think I did anything to affect this, probably another bug.

DanielRosenwasser · 2025-06-16T17:29:59Z

I'm also not setting up maxNodeModuleJsDepth for the the editor. I'm not sure where that goes.

That was previously set for InferredProjects that have at least one JavaScript file. I'm not sure if it is now.

jakebailey · 2025-06-16T17:52:45Z

Theoretically, the tsconfig parser will set this default for jsconfig files, but that path does not appear to be used for the server, which has its own inferred options. In the new code, createInferredProject is the thing to modify, but I'm not sure how to get "there's a JS file" at that point; it's too early?

DanielRosenwasser · 2025-06-16T23:26:58Z

internal/compiler/fileloader.go

+		if imp.Kind == ast.KindStringLiteral {
+			moduleNames = append(moduleNames, imp)
+		}
+		// Do nothing if it's an Identifier; we don't need to do module resolution for `declare global`.


I really dislike how global augmentations are module augmentations with an identifier.

Hey I just work here

DanielRosenwasser · 2025-06-16T23:28:37Z

internal/compiler/fileloader.go

-			resolutionsInFile[module.ModeAwareCacheKey{Name: resolution.node.Text(), Mode: mode}] = resolution.resolvedModule
+			resolvedFileName := resolvedModule.ResolvedFileName
+			isFromNodeModulesSearch := resolvedModule.IsExternalLibraryImport
+			// If this is js file source of project reference, dont treat it as js file but as d.ts


I know this is from an existing comment, but can we fix it up so it's clearer what the intent is?

DanielRosenwasser · 2025-06-16T23:30:20Z

internal/compiler/fileloader.go

+			isFromNodeModulesSearch := resolvedModule.IsExternalLibraryImport
+			// If this is js file source of project reference, dont treat it as js file but as d.ts
+			isJsFile := !tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedTSExtensionsWithJsonFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil
+			isJsFileFromNodeModules := isFromNodeModulesSearch && isJsFile && (resolvedFileName == "" || strings.Contains(resolvedFileName, "/node_modules/"))


When is resolvedModuleName the empty string?

Probably never, and the original code just had string | undefined for a failed resolution. I just directly ported it, but could change it.

DanielRosenwasser · 2025-06-16T23:35:10Z

internal/compiler/fileloader.go

-			}
-			shouldAddFile := resolution.resolvedModule.IsResolved() && hasAllowedExtension
-			// TODO(ercornel): !!!: other checks on whether or not to add the file
+			shouldAddFile := moduleName != "" &&


When is moduleName supposed to be "" other than when the user wrote that?

Also- do we resolve modules based on module augmentations? That's how I'm reading this code.

Module name shouldn't be the empty string unless the user wrote it, yes.

We do resolve modules based on augmentation, it's just that augmenting a module and then failing to resolve that module is legal, as the augmentation would have no effect.

DanielRosenwasser · 2025-06-16T23:40:17Z

internal/compiler/fileloader.go

+// Returns a DiagnosticMessage if we won't include a resolved module due to its extension.
+// The DiagnosticMessage's parameters are the imported module name, and the filename it resolved to.
+// This returns a diagnostic even if the module will be an untyped module.
+func getResolutionDiagnostic(options *core.CompilerOptions, resolvedModule *module.ResolvedModule, file *ast.SourceFile) *diagnostics.Message {


Is this only written this way because future code will eventually need the diagnostic?

Yes, this code is supposed to be used in the checker, but for some reason is currently not.

DanielRosenwasser · 2025-06-16T23:54:18Z

internal/compiler/fileloadertask.go

+			}
+
+			if currentDepth < loadedTask.lowestDepth {
+				// If we're seeing this task at a lower depth than before,


How does this happen?

Imagine:

A -> B -> C -> D -> E \---------^

If we walk A-B-C-D first and D exceeds the max depth (so is skipped, we'll eventually walk through B instead and have to redo D, and and then E, and so on.

DanielRosenwasser · 2025-06-17T00:09:14Z

internal/compiler/fileloader.go

+			shouldAddFile := moduleName != "" &&
+				getResolutionDiagnostic(optionsForFile, resolvedModule, file) == nil &&
+				!optionsForFile.NoResolve.IsTrue() &&
+				!(isJsFile && !optionsForFile.GetAllowJS()) &&


Suggested change

!(isJsFile && !optionsForFile.GetAllowJS()) &&

(!isJsFile || optionsForFile.GetAllowJS()) &&

This is a direct port, but I could change it.

DanielRosenwasser · 2025-06-17T00:12:13Z

internal/compiler/fileloader.go

+			resolvedFileName := resolvedModule.ResolvedFileName
+			isFromNodeModulesSearch := resolvedModule.IsExternalLibraryImport
+			// If this is js file source of project reference, dont treat it as js file but as d.ts
+			isJsFile := !tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedTSExtensionsWithJsonFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil


Why isn't this just the following? Just a faithful translation?

Suggested change

isJsFile := !tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedTSExtensionsWithJsonFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil

isJsFile := tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedJSExtensionsFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil

Yes, all of this is a faithful port.

jakebailey · 2025-06-17T07:29:32Z

...nes/reference/submoduleAccepted/conformance/checkExportsObjectAssignProperty.errors.txt.diff

+     Object.defineProperty(module.exports, "thing", { value: "yes", writable: true });
+                          ~~~~~~
+!!! error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i --save-dev @types/node`.
+     Object.defineProperty(module.exports, "readonlyProp", { value: "Smith", writable: false });


Seemingly this is broken because the new code does not yet handle defineProperty based exports. But, it's not clear to me why this succeeded in the past.

jakebailey added 9 commits June 13, 2025 17:53

Simplify code

e94211c

Fix noResolve

72edd5d

Fix loader generics

30ff966

Starting tracking of node modules depth

053af5b

it works

f13dffc

Skip unrun

f8305a6

cleanup

4ebaa9f

Tweak

2d84c60

rename

06f94ed

jakebailey requested review from andrewbranch, sheetalkamat and Copilot June 14, 2025 21:10

jakebailey mentioned this pull request Jun 14, 2025

Some dependency JS files appear to be erroneously typechecked #931

Open

Copilot AI reviewed Jun 14, 2025

View reviewed changes

jakebailey added 3 commits June 14, 2025 14:14

fmt

05245c3

lint

9f0f9cc

remove comment

0f12bac

jakebailey marked this pull request as draft June 15, 2025 04:12

More directly port

0d68348

jakebailey commented Jun 15, 2025

View reviewed changes

jakebailey added 3 commits June 14, 2025 21:26

doc

fdf7e30

More accurately port isAnExternalModuleIndicatorNode

3145cc5

more exactly match old depth behavior

b0b5380

jakebailey marked this pull request as ready for review June 15, 2025 05:33

jakebailey added 2 commits June 14, 2025 22:39

Closer to original

19e90bc

Formatting

a117059

jakebailey mentioned this pull request Jun 16, 2025

Fix code that should have used IsRequireCall #1195

Merged

jakebailey mentioned this pull request Jun 16, 2025

"invalid memory address or nil pointer dereference" #1178

Open

Merge branch 'main' into jabaile/node-modules-depth

ee8c8f6

jakebailey mentioned this pull request Jun 16, 2025

Cannot get types for require(...) style imports with tsgo (but can with regular ts) #1181

Open

DanielRosenwasser reviewed Jun 16, 2025

View reviewed changes

DanielRosenwasser reviewed Jun 17, 2025

View reviewed changes

jakebailey commented Jun 17, 2025

View reviewed changes

		+index.js(10,12): error TS2749: 'Thing' refers to a value, but is being used as a type here. Did you mean 'typeof Thing'?
		+index.js(17,16): error TS2749: 'Thing' refers to a value, but is being used as a type here. Did you mean 'typeof Thing'?

	!(isJsFile && !optionsForFile.GetAllowJS()) &&
	(!isJsFile \|\| optionsForFile.GetAllowJS()) &&

	isJsFile := !tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedTSExtensionsWithJsonFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil
	isJsFile := tspath.FileExtensionIsOneOf(resolvedFileName, tspath.SupportedJSExtensionsFlat) && p.projectReferenceFileMapper.getRedirectForResolution(ast.NewHasFileName(resolvedFileName, p.toPath(resolvedFileName))) == nil

Implement maxNodeModuleJsDepth, noResolve #1189

Are you sure you want to change the base?

Implement maxNodeModuleJsDepth, noResolve #1189

Conversation

jakebailey commented Jun 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakebailey commented Jun 16, 2025

Uh oh!

DanielRosenwasser Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielRosenwasser commented Jun 16, 2025 •

edited

Loading

DanielRosenwasser Jun 16, 2025 •

edited

Loading

jakebailey Jun 17, 2025 •

edited

Loading