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.

Sign up for GitHub

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

Fix missing error output in root project with project references #937

Merged
merged 3 commits into from
May 19, 2019
+17 −14
Merged

Fix missing error output in root project with project references #937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
446cd74
Fix missing error output in root project with project references
andrewbranch May 18, 2019
ccf83e0
Update CHANGELOG
andrewbranch May 18, 2019
69dc5e2
Dedupe “output file has not been built from source file” diagnostic m…
andrewbranch May 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## v6.0.1

* [Fix issue with `resolveTypeReferenceDirective` causing errors like `Cannot find name 'it'` with Jest](https://github.com/TypeStrong/ts-loader/pull/936) (#934) (#919) - thanks @andrewbranch!
* [Fix TypeScript diagnostics not being printed to console when using project references](https://github.com/TypeStrong/ts-loader/pull/937) (#932) - thanks @andrewbranch!

## v6.0.0

Expand Down
30 changes: 16 additions & 14 deletions src/after-compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as path from 'path';
import * as ts from 'typescript';
import * as webpack from 'webpack';

import * as constants from './constants';
Expand All @@ -12,6 +13,7 @@ import {
} from './interfaces';
import {
collectAllDependants,
ensureProgram,
formatErrors,
isUsingProjectReferences
} from './utils';
Expand Down Expand Up @@ -174,8 +176,6 @@ function provideErrorsToWebpack(
) {
const {
compiler,
program,
languageService,
files,
loaderOptions,
compilerOptions,
Expand All @@ -187,13 +187,14 @@ function provideErrorsToWebpack(
? constants.dtsTsTsxJsJsxRegex
: constants.dtsTsTsxRegex;

// I’m pretty sure this will never be undefined here
const program = ensureProgram(instance);
Copy link
Member

Choose a reason for hiding this comment

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

I trust you dude 😁

for (const filePath of filesToCheckForErrors.keys()) {
if (filePath.match(filePathRegex) === null) {
continue;
}

const sourceFile =
program === undefined ? undefined : program.getSourceFile(filePath);
const sourceFile = program && program.getSourceFile(filePath);

// If the source file is undefined, that probably means it’s actually part of an unbuilt project reference,
// which will have already produced a more useful error than the one we would get by proceeding here.
Expand All @@ -203,16 +204,17 @@ function provideErrorsToWebpack(
continue;
}

const errors =
program === undefined
? [
...languageService!.getSyntacticDiagnostics(filePath),
...languageService!.getSemanticDiagnostics(filePath)
]
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, can you recall why we used to alternate between languageService and program here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, these lines weren't the issue, I just updated them for consistency with the changes above, which were the issue.

languageService.get*Diagnostics() essentially just calls its internal program.get*Diagnostics(). Based on loader options (and I honestly can't remember what options produce what scenarios), we either have a program directly (instance.program) or a language service which has a program under the hood (instance.languageService.getProgram()). Before, we were branching here in this function based on where the underlying program is. But, we already have a util—ensureProgram—that does that branching.

TL;DR: these lines are basically equivalent because of ensureProgram; the lines above should have been looking at both program and languageService but were only looking at program.

: [
...program.getSyntacticDiagnostics(sourceFile),
...program.getSemanticDiagnostics(sourceFile)
];
const errors: ts.Diagnostic[] = [];
if (program && sourceFile) {
errors.push(
...program!.getSyntacticDiagnostics(sourceFile),
...program!
.getSemanticDiagnostics(sourceFile)
// Output file has not been built from source file - this message is redundant with
// program.getOptionsDiagnostics() separately added in instances.ts
.filter(({ code }) => code !== 6305)
);
}
if (errors.length > 0) {
const fileWithError = files.get(filePath) || otherFiles.get(filePath);
filesWithErrors.set(filePath, fileWithError!);
Expand Down