Skip to content
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

Assist: ts-loader#945 #1

Closed

Conversation

andrewbranch
Copy link

This should, I think, take care of the remaining test failures in TypeStrong#945. I'll leave a review to explain the changes inline.

dependabot bot and others added 7 commits June 10, 2019 10:42
- track which files have been renamed by appendSuffixTo and exclude them from needing to be in rootFileNames
- check for project references before rootFileNames
- fix up remaining test tsconfigs
Copy link
Author

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

@davazp thanks so much for the awesome work so far! Let me know what you think about this.

@@ -139,3 +140,49 @@ export function getConfigParseResult(

return configParseResult;
}

function getSuffixAppendingParseConfigHost(
Copy link
Author

Choose a reason for hiding this comment

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

This is the heart of my changes. It solves the appendSuffix problem by providing TypeScript an interface to the file system that actually acts on the file names that ts-loader tracks, i.e. renamed with the .ts or .tsx suffix appropriately. The result is that configParseResult.fileNames will include things like foo.vue.tsx when ts-loader is told to assume foo.vue is a TSX file.

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.

Au contraire mon frère, @johnnyreilly 😄

Copy link
Owner

Choose a reason for hiding this comment

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

This is the heart of my changes. It solves the appendSuffix problem by providing TypeScript an interface to the file system that actually acts on the file names that ts-loader tracks

That's clever! But could this be a bit unintuitive for the users? Some patterns would match actual files but not transformed files. For instance, patterns like src/**/*.vue, or just main.vue inside "files". Right?

Of course the user can specify patterns on the transformed files. But then the appendSuffix is not that transparent anymore. Maybe that's not wrong, but still pretty confusing. 🤔

);
}
}

Copy link
Author

Choose a reason for hiding this comment

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

Moved this to come after project reference handling

@@ -178,28 +171,23 @@ function successLoader(
*/
function reloadRootFileNamesFromConfig(
loaderContext: webpack.loader.LoaderContext,
instance: TSInstance
instance: TSInstance,
options: LoaderOptions
Copy link
Author

Choose a reason for hiding this comment

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

instance.loaderOptions !== options, ouch 😖 Pretty sure we do this wrong in lots more places, but this one was needed to get tests passing

@@ -432,6 +421,9 @@ function updateFileInCache(
//
if (!instance.rootFileNames.has(filePath)) {
instance.version!++;
if (options.onlyCompileBundledFiles) {
instance.rootFileNames.add(filePath);
Copy link
Author

Choose a reason for hiding this comment

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

Need to add the file manually, otherwise rootFileNames will just be declarations

@@ -96,8 +96,7 @@ export function makeServicesHost(

getProjectReferences: () => projectReferences,

getScriptFileNames: () =>
[...files.keys()].filter(filePath => filePath.match(scriptRegex)),
getScriptFileNames: () => Array.from(instance.rootFileNames),
Copy link
Author

Choose a reason for hiding this comment

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

This was the change I mentioned before that fixes TypeStrong#949. Doing this exposed a couple other problems, like needing to add the file to instance.rootFileNames when onlyCompileBundledFiles is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants