Can we put the new functionality behind a loader option please? Something like usePreviousLoaderGeneratedFiles. (Maybe something shorter?) I don't want this to run unless people have "flicked the switch".

So calling getGeneratedFiles etc only happens when you're using usePreviousLoaderGeneratedFiles

No problem.

The option usePreviousLoaderGeneratedFiles has been implemented in the latest commit.

Btw. I am happy with the option name usePreviousLoaderGeneratedFiles, but say the word, and I will change it.

Is it significant that it's not passed down to getTranspilationEmit but is to getEmit?

It is only in getEmit that dependencies are handled. Dependencies should not be added for generated files. For example a dependency should not be added for Game.css.d.ts if it is a generated file. We rely on the generator to add any dependencies, that are needed. In this example the generator (ts-css-loader) would add a dependency on Game.css, not Game.css.d.ts witch it generates.

Hmm... I see that the above may not be entirely clear. The reason getEmit recieves generatedFiles is that it adds dependencies, and they need to be filtered so that generated files are excluded. getTranspilationEmit does not add dependencies and so does not need generatedFiles.

I could easily move the filtering of dependencies out of getEmit. That would also make it easy to restrict it to those cases when generated files are enabled.

Filtering of dependencies has been moved to new function removeGeneratedFileDependencies in the latest commit. The call to this function is, of course, only made if the usePreviousLoaderGeneratedFiles option is true.

Actually removeGeneratedFileDependencies has become removeDependencies, and is also used in getEmit instead of clearDependencies.

Is this commented for a reason? I think this is necessary.

I commented it out because I am a bit unsure. But if loader.clearDependencies is called any dependencies added in previous loaders in the chain will be forgotten, which would require som jumping through hoops to handle.
I may be missing something, but I don't see that it is necessary to make this call. Normally (if there are no previous loaders in the chain) there would only be one file dependency - on the ts-file being loaded. Or am I misuderstanding something?

I think what is really wanted is not to clear dependencies, but to replace the original dependency on resourcePath with a dependency on rawFilePath.
My latest commit implements this while preserving any other existing dependencies.

Actually having "[built]" is significant I believe. It indicates whether a module has been recompiled as part of a run. I believe this is important for the integration tests which have multiple iterations.

Well, in the previous PR, and in the first integration test of this PR, I saw that one test failed, but only for Node 4, because [build] was not there. However the test succeeded for all other versions of Node and TypeScript.

The test in question is aliasResolution. See Job #1484.6

This also happens sometimes in nightly builds: #1473.20

Oh, and I made a mistake by filtering out [build] in stead of [built], but the second integration test of this PR still passed. So there must be something, unrelated to this PR, that makes webpack output [built] where it is not expected on seldom occasions, but I am at a loss as to what that something is.

Hey! The first thing to say is that the integration tests are flaky. As it's a full integration test you do sometimes get false failures. This happens during the nightly builds. I've never looked into it in detail but generally rerunning a failing test pack will result in a green run. Anyway, [built] is significant AFAIK as it indicates whether a module has been recompiled as part of a run.

So I'd take the occasional failure here as a "something to live with". I think we need this or we may break behaviour in ts-loader in future and not realise it.

Right - I'll roll back that change then.

This looks unrelated to the change being made - could you provide some info on this please?

When a file is generated by a previous loader in the chain, it does not necessarily exist in the file system. So to make sure the TypeScript compiler can resolve the file, I look up in intance.files first. Otherwise it would not be able to resolve generated files. This should actually give a slight performance improvement, as files will only be loaded once (and of course maybe in watch-run).

Cool - could we document that in comments please? Perhaps it would be worth this code path only being executed if usePreviousLoaderGeneratedFiles is true; presumably otherwise it would not work if there weren't previous loaders in the chain?

I'll add some comments.

However I don't think it is necessary to wrap the code with a check for usePreviousLoaderGeneratedFiles. The files object is the one already used in ts-loader to cache files. Its contents are loaded in the following places:

• From the file system initially in getTypeScriptInstance:

const files: interfaces.TSFiles = {};
...
const filesToLoad = configParseResult.fileNames;
filesToLoad.forEach(filePath => {
	normalizedFilePath = path.normalize(filePath);
	files[normalizedFilePath] = {
		text: fs.readFileSync(normalizedFilePath, 'utf-8'),
		version: 0
	};
  });

• In the loader itself, using updateFileInCache (I've excluded my additional calls to updateFileInCache for this discussion):

const rawFilePath = path.normalize(this.resourcePath);
const filePath = utils.appendTsSuffixIfMatch(options.appendTsSuffixTo, rawFilePath);
const fileVersion = updateFileInCache(filePath, contents, instance);

• In watch-run.ts where file changes are registered

So it should be safe enough to load a file from files and use that, if it exists, instead of reading the file from the file system. This should even result in a slight performance improvement.

I would actually recommend updating files after reading them from the file system, and being able to have text == undefined for those files, that do not exist. This would reduce the file system access to once per file (initially - watch-run can of cause re-read any number of files).

All uses of file.text should be checked so they can handle file.text == undefined.

I am very happy to make this change - either in this PR or a new one, if you like.

There is one fly in the ointment. When I look at watch-run.ts, I see:

file.text = utils.readFile(filePath) || '';

This means that files that no longer exist are not deleted and have a valid text.

If I make the changes I recommend above I would change this to:

file.text = utils.readFile(filePath);

All seems reasonable. If existing tests pass with those changes then I don't have any objection. I'm guessing performance would not be hurt by this?

Quite the contrary :-) I'll get to these changes within the next couple of days (I am actually at my day job right now).

I have added comments to moduleResolutionHost. I have decided not to make the suggested changes to file caching though, as I think that deserves its own PR, and shouldn't delay this one.

Could you provide some explanation on what is happening here? This replaces the simple loader.clearDependencies() with something that will clear dependencies and then perhaps add dependencies again. I'm not clear on what loader.resourcePath is and whether we always want to follow this path.

Some details of what this should achieve would be helpful. Also, we still have the addDependency call after removeDependencies - will this result in double adds?

I think maybe you missed the comments on outdated changes (probably my fault, not being an experienced GitHub user). But the gist is this:

The problem with calling clearDependencies is that it clears dependencies for the entire chain, not just the current loader. So if a loader has been run before ts-loader, any dependencies it has added will be cleared (in the case of ts-css-loader this could be dependencies on .css files).

Normally, if there are no previous loaders in the chain, there would only be one file dependency: the ts-file being loaded (resourcePath).

I think what is really wanted is not to clear dependencies, but to replace the original dependency on resourcePath with a dependency on rawFilePath, which is defined like so:

const rawFilePath = path.normalize(this.resourcePath);

That's why I replaced clearDependencies with removeDependencies and addDependency.

Of course, I should check if rawFilePath === this.resourcePath, and if so skip this step entirely.

To be honest I think the GitHub UI is making things more confusing than I would hope 😄

I'm still a little confused and at least part of that is down to my incomplete knowledge of the webpack API. (Just for history, ts-loader was originally written by @jbrantly and I'm the main caretaker at present.) I'll lay out my confusion and you can set me straight.

As I read it, removeDependencies does the following:

• it gathers up existing contextDependencies and dependencies from webpack.
• clears dependencies
• re-adds the previously gathered dependencies unless they are the resourcePath - could you clarify what the intent of this is please? I can see what's happening but I'm not so clear as to what you want to not be added as a dependency and what you do.

See my comment in the changes tab.

Well my understanding is that the goal of the original call to clearDependencies is to replace the dependency on resourcePath with rawFilePath, because it might be different.

To understand what is going on, I have looked in webpack/loader-runner where dependencies for loaders are handled, and f.ex. addDependency and clearDependencies are implemented.

It turns out that there is no method that will remove individual dependencies. The only method that actually removes dependencies is clearDependencies.

There is a method getDependencies that returns file dependencies, but it returns a copy of the internal dependency array, so any changes to will have no effect. The method getContextDependencies works in the same way.

So, if I want to replace a dependency, I have to get the existing dependencies, call clearDependencies, add all the dependencies except the one I want to replace, and then add the new dependency.

It turns out that clearDependencies clears file and context dependencies, so I have to do the same for context dependencies.

NB. I have just checked the code in webpack/loader-runner again, and my assumption that, if ts-loader is the only loader, resourcePath will be the only dependency, is correct.
So the only reason, I can think of, for the original call to clearDependencies is to replace resourcePath with the same path, but normalized.

OK cool that makes sense. So given we weren't preserving dependencies / contextDependencies in our original code it wasn't an issue because we would only be dealing with a single file in that context. But this is not true in the world where css-ts-loader is chained before ts-loader. Is that correct?

Exactly!

I wonder whether replacing the dependency on resourcePath is really necessary. Even if it isn't normalized it should still trigger a new webpack build if the file is changed. Otherwise webpack would have problems with other loaders. The only place I can think of where it would be an issue is in watch-run.ts, but it already normalizes the paths of changed files.

I'm basically happy with this - I'd just like to do some variable renames to improve clarity. Please could

• dependencies become dependenciesToPreserve
• contextDependencies become contextDependenciesToPreserve

Bit of a mouthful but it makes intent obvious

Done

Could filePath be renamed to something so people don't mix it up with the filePath in the method signature for exclude please?

Identifier filePath has become dependency.