Throw error if a file not in the TS project is found

[davazp]

This pull request keeps track of the project's root filenames and uses it to increase the instance.version when required, solving #943.

Additionally, it will throw an error like

Error: The file /Users/davazp/Projects/.../analytics.ts is not part of the project specified in tsconfig.json. Make sure it is covered by the fields 'include', 'exclude' and 'files'.
    at successLoader (/Users/davazp/Projects/ts-loader/dist/index.js:70:19)
    at Timeout.retry [as _onTimeout] (/Users/davazp/Projects/ts-loader/dist/index.js:35:13)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)

when such a file is found and onlyCompileBundledFiles is false. This second behaviour is not mandatory to solve the issue, but I think it is desirable to ensure the project would work with tsc as well by default.

Summary of changes:

• Add rootFileNames to TSInstance. This property gets initialized when the instance is created and updated on new files.

• Add configFile and configFilePath to TSInstance to make sure the same snapshot of tsconfig is always used to get the new list of root files.

• On new root files, call reloadRootFileNamesFromConfig which will recall getConfigParseResult, and so it will read files from the project, and use it to update rootFileNames.

• If a file passed to the loader is not a root file name, and onlyCompileBundledFiles is false, throw an error. As this file is not covered by tsconfig.json.

• If a new root file name is found, increase the instance version, to solve Non-deterministic error when compiling files outside of the TS project #943 .

[davazp]

Some tests broke because they indeed had some files not covered by tsconfig.json. Would it make sense to introduce a flag to disable this check?

I think it is still useful but, otherwise we can remove the check all together and just use rootFileNames to increase instance.version, fixing the original bug.

[johnnyreilly]

Could you fix the linting issues please? CI won't run the tests until these are resolved.

[johnnyreilly]

Some tests broke because they indeed had some files not covered by tsconfig.json. Would it make sense to introduce a flag to disable this check?

Before we do that, can we drill into which tests are failing please?

I think it is still useful but, otherwise we can remove the check all together and just use rootFileNames to increase instance.version, fixing the original bug.

I didn't quite follow your meaning here - could you elaborate please?

[davazp]

Many tests fail just because they specify some "files" in tsconfig.json but not all of them. The expected case that the code is supposed to report. If I add the files, the errors are gone (the few I tried). I can go through them and fisx the tsconfig.json files and see what is left.

Another case of failing test isappendSuffixTo. The files are not in tsconfig.json again, but it fails if I add them, then it complains about the file not existing.. so this seems a related but different bug.

I think it is still useful but, otherwise we can remove the check all together and just use rootFileNames to increase instance.version, fixing the original bug.

I didn't quite follow your meaning here - could you elaborate please?

So to recap, if a file is imported from a TS file but it's not part of the project, webpack will not emit code when that file is passed later on to the loader.

This line fixes that issue:
https://github.com/TypeStrong/ts-loader/pull/945/files#diff-f41e9d04a45c83f3b6f6e630f10117feR433

However, I also implemented the check for when a file is not covered by tsconfig.json at all, giving a more helpful (and deterministic) error. I think it is a desirable thing. It's a breaking change, but users can always go and correct their tsconfig.json. But if we prefer not to break users setup in any way, then we can make it under a flag or disable the check completely, but keeping the basic fix.

[davazp]

I fixed most comparison tests.

Still 3 failing:

• nodeModulesMeaningfulErrorWhenImportingTs
• appendSuffixToWatch
• appendSuffixTo

I'll have a look to the rest the next days.

[johnnyreilly]

Another case of failing test is appendSuffixTo. The files are not in tsconfig.json again, but it fails if I add them, then it complains about the file not existing.. so this seems a related but different bug.

So I think this is actually a case where we can reliably be certain that the files that exist will not be in the tsconfig.json and in fact cannot be.

This feature (appendSuffixTo etc) exists to facilitate non TypeScript files in the build to which .ts will be suffixed. The most typical case is Vue files (which have the suffix .vue I think)

It looks like your proposed change conflicts with this use case. Thoughts?

PS I'm afraid I'm going to be a bit stop/start in my interactions here. Life is super busy right now. Please bear with me

[davazp]

So I think this is actually a case where we can reliably be certain that the files that exist will not be in the tsconfig.json and in fact cannot be.

That sounds correct.

I'll have a better look at this functionality and see if something can be done. We can, of course, disable the check if that functionality is used. It's kind of saying that appendSuffixTo implies onlyCompileBundledFiles.

But I will check if there are other options.

PS I'm afraid I'm going to be a bit stop/start in my interactions here. Life is super busy right now. Please bear with me

Don't worry. Take your time, there is no rush!

[andrewbranch]

WOOP, thank you so much for this! I was just about to do the same thing, and then I discovered that you had already done a better, more thorough version of what I was going to do, which really makes my Friday. 😍

1. Just tested this, and if you update getRootFileNames and getScriptFileNames in serviceHost.ts to return Array.from(instance.rootFileNames), this fixes Incorrect getScriptFileNames/getRootFileNames causes resolution bugs #949 🎉
2. Small nit, but looks like there must have been some spaces/tabs confusion in the test tsconfig.json changes that make the diffs a little rough to read... sadly prettier doesn’t support JSON files. Would you mind blasting them with some kind of formatter real quick just to ensure spacing is consistent at least on a per-file basis?
3. Incorrect getScriptFileNames/getRootFileNames causes resolution bugs #949 is blocking me from testing Using Solution builder to build project references #935, so my top priority Monday will be to help you get this in if you haven’t done so by then. Anything I can do to help?

[andrewbranch]

FYI I'm working on a PR to this PR to handle the remaining failing tests 👍

[johnnyreilly]

See my comment in the resolved conversation above @andrewbranch. Tl;Dr - we could remove that option if it's best

[andrewbranch]

Not necessary at the moment, just gathering context and making notes for the future 👍

[andrewbranch]

In related questions: have you ever published a prerelease version of ts-loader? Sometimes I worry a bit about these big changes in environments that are hard to test, like watch mode where files are being incrementally changed, added, and removed. Do you think ts-loader’s audience is large enough that we’d get valuable feedback in a prerelease version?

[johnnyreilly]

have you ever published a prerelease version of ts-loader? Sometimes I worry a bit about these big changes in environments that are hard to test, like watch mode where files are being incrementally changed, added, and removed. Do you think ts-loader’s audience is large enough that we’d get valuable feedback in a prerelease version?

I'm happy for us to try. I have done it before, and whilst I got some feedback, alas most problems were found post-release.

But as I say - feel free to give it s whirl. TBH there's always going to be nervousness around this stuff. I think I'm generally of the view that because you never get all the feedback you need, it's better to push ahead and fix it in post if there are problems. Be careful but be bold 😊

[andrewbranch]

I've opened a PR to the PR and explained my updates: davazp#1

This gets tests passing for me locally, always a mystery whether they’ll pass in AppVeyor 😛

[johnnyreilly]

Yeah the comparison tests are so brittle.... that's why we only run them against the latest TypeScript version and prefer the execution tests wherever possible!

[davazp]

I feel the best way is to say that appendSuffixTo is incompatible with onlyCompileBundledFiles=false .

I think that makes sense with the description of onlyCompileBundledFiles:

The default behavior of ts-loader is to act as a drop-in replacement for the tsc command, so it respects the include, files, and exclude options in your tsconfig.json, loading any files specified by those options.

If you want to stick to tsconfig.json, then appendSuffixTo shouldn't be allowed.

If onlyCompiledBundledFiles=true, then appendSuffixTo option sounds reasonable.

[andrewbranch]

I personally like that constraint, but do you think it will cause riots in the streets? I always had trouble getting onlyCompileBundledFiles to work for me, so if I were also relying on appending suffixes, I would be in bad shape if I had to take them together.

[davazp]

I personally like that constraint, but do you think it will cause riots in the streets?
It could be. I can imagine large projects combining Typescript and vue. It would be a pity that the whole project needs onlyCompileBundledFiles for only some vue files.

We can always allow files matched by appendSuffixTo, regardless if they are covered by tsconfig.json or not. It's not very intuitive, but it is compatible.

Or we can allow new root files completely as now, but with the missing instance.version++.

You guys can judge this better.

[davazp]

Any take on this then?

I fix the bug only with the backward compatible change for now and keep the discussion open with the change to throw errors on unknown files.

[johnnyreilly]

Ping @andrewbranch 😁

@davazp did you merge the PR to your PR that Andrew?

[davazp]

@davazp did you merge the PR to your PR that Andrew?

Not yet. I wanted to wait until we decided.

[andrewbranch]

Sorry, I’ve been slammed this week. I buy the critique of my PR, but at the same time, this PR as it stands still has several failing tests. I think making incremental changes along these lines (e.g. doing whatever is easiest with appendSuffixTo for now) sounds good, but obviously we need to get to a green build first. I probably won’t have time to work on it much until next week.

[johnnyreilly]

Don't worry about the delay - nothing is burning here 😁

Just wanted to see where things were. Thanks for responding!

[davazp]

I'll open a PR with the small fix. That should be completely compatible and shoudn't break the build 👍

[davazp]

I guess we should close this PR if we won't push more for it?

[johnnyreilly]

I'm not sure if there's more to be done here or not actually... @andrewbranch do you have a view?

[andrewbranch]

Oh, I thought this had been merged 😅

[johnnyreilly]

Ha! 😁

Where does this leave things gents?

[andrewbranch]

IIRC it looks good generally, but had legitimately failing tests

[andrewbranch]

I believe I opened a PR to try to fix those, did fix most of them, but was also overzealous and made a couple related changes (fixing #949) that ultimately we agreed would be better to revisit separately.

[johnnyreilly]

Okay. It looks like there's a bunch of failing tests eg 3.0.1_projectReferences. And they're execution tests as well, so they represent failing functionality.

Also, the branch is rather out of date and there's conflicts.

If someone is up for catching this branch up and working on the test failures that'd be great. If not we should probably close this.

[johnnyreilly]

Side note: it looks like typescript-eslint has just implemented similar functionality to improve the performance of ESLint: typescript-eslint/typescript-eslint#760

Thought I'd share as it's related

[davazp]

We merge a fix for a bug caused by files not in the project.

To recap, one of the issues we found with this functionality is that it conflicts with other features (like support for vue files, which can't be part of a project!).

Having a project flag to enable the feature is possible. There are still conflicting options, but we can say they are exclusive.

[andrewbranch]

Oh right, I was thinking of #955 when I was thinking that this had been merged. So, I’m still not clear exactly what advantage this PR has on top of what was already fixed. I recall the issue with Vue files, but I’m not sure what the status quo is post-#955 compared to what it would be with this PR merged.

[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.

[stale]

Closing as stale. Please reopen if you'd like to work on this further.