Incorrect getScriptFileNames/getRootFileNames causes resolution bugs

[andrewbranch]

I'm not even going to try to figure out a minimal repro, but I'm observing it happening in a real-world project while testing #935. Basically, this

ts-loader/src/servicesHost.ts

Line 100 in d677d73

[...files.keys()].filter(filePath => filePath.match(scriptRegex)),

is supposed to return only the root file names, i.e., what TypeScript sees as “entrypoints,” i.e., excluding libs and imported modules that aren’t covered by include or files in the tsconfig.

However, what it’s doing here is returning all the script files Webpack knows about, in no particular order. This is a problem because later on, TypeScript will try to merge this array of files with more files, and the resulting order is important. However, since this array already contains all the other things TypeScript is trying to merge in, it’s a no-op and that important ordering never happens.

The particular symptom I’m observing is that, with transpileOnly: false, my project compiles cleanly in the initial run, but when I modify a file that assumes the return value of setTimeout is a number (which it is in the browser), I get errors because the type checker thinks setTimeout is coming from @types/node (which returns an object).

What’s happening under the hood is that on the first run, ts-loader isn’t yet aware of any lib files, so getScriptFileNames() works, and TypeScript sorts lib files and typings files appropriately, such that the symbol for setTimeout has a valueDeclaration coming from lib.dom.d.ts, and some other declarations from @types/node. But on the second run, ts-loader’s instance.files contains lib files and typings for Node, and the ones for Node happen to come first, so that becomes the valueDeclaration for the global symbol setTimeout.

I actually think #348 might have been caused by this.

At any rate, I’m going to fix it. Just wanted to document it.

[andrewbranch]

Wait, this might fix #943 too

[andrewbranch]

Or better, yet #945 might fix this

[andrewbranch]

Yessss, #945 is 99% of the way there: #945 (review)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[andrewbranch]

Thanks for calling me out, stale-bot 🙊

[johnnyreilly]

Ha! I pinned on the assumption this was on your list 😄

[JoostK]

Oh hey, I have a local patch for this exact bug dating back to december 2018. I just never got around to reporting an issue here. I recall having debugged this a bit back then, after which my intent was to open a PR to fix this issue. The weird thing was that I was unable to reproduce in ts-loader tests, with what I believe to have been the exact setup that was failing for me in real life.

This started to be a problem once enabling project references. Module imports into source files of referenced projects would be cached in the files map and from there on incorrectly be considered root files. This is a problem, as these files would occur outside of rootDir (IIRC it's essential for this to be set).

I am willing to contribute a PR, however I had a lot of trouble with running the tests at the beginning of this year. I experienced assertion failures in OS X, so had to switch into a Docker image to be able to run tests which made the whole process quite painful. I would appreciate any help :-)

--- a/servicesHost.js
+++ b/servicesHost.js
@@ -28,10 +28,11 @@ function makeServicesHost(scriptRegex, log, loader, instance, enableFileCaching,
     const clearCache = enableFileCaching ? addCache(moduleResolutionHost) : null;
     // loader.context seems to work fine on Linux / Mac regardless causes problems for @types resolution on Windows for TypeScript < 2.3
     const getCurrentDirectory = () => loader.context;
+    const rootFiles = [...files.keys()].filter(filePath => filePath.match(scriptRegex));
     const servicesHost = {
         getProjectVersion: () => `${instance.version}`,
         getProjectReferences: () => projectReferences,
-        getScriptFileNames: () => [...files.keys()].filter(filePath => filePath.match(scriptRegex)),
+        getScriptFileNames: () => rootFiles,
         getScriptVersion: (fileName) => {
             fileName = path.normalize(fileName);
             const file = files.get(fileName);

(Applies to 5.3.3, may not be up to date with latest release).

BTW, I do have an additional patch for dealing with recursive project references. That feature is dependent on TS 3.2 which had not yet been released when ts-loader added support for project references, but support appears to be missing still.

[johnnyreilly]

Hey @JoostK!

Thanks for commenting. I'll let @andrewbranch respond on the project references bits and pieces as he certainly knows more than me about that.

Regarding your fix and the tests I'd say this: initially at least don't worry about tests. Raise a PR explaining context / motivation etc and let's see what the failing tests look like.

There's 2 sets of integration tests that ts-loader runs and, being integration tests, they can be slow and somewhat flaky. If you raise the PR I can look at the nature of the failures and advise. If it's comparison tests that are failing we may want to regenerate the output, assuming we're happy with the changed behaviour. Or it might be legitimate problems being caught. Either way, there's a way forward from that. Hope that helps 🤗

[andrewbranch]

Cool! But unfortunately, there’s a case this doesn’t cover: adding a new root file or deleting a root file while in watch mode. getScriptFileNames needs to be dynamic because the list of root files might legitimately change. But as you pointed out, non-root files find their way into files too. I think we need to split the catch-all files into two separate buckets. I also got stuck on figuring out how to treat non-TS extensions (like Vue), though I don’t remember the immediate problem that had to be solved.