-
Notifications
You must be signed in to change notification settings - Fork 13k
Apply --checkjs to bind diagnostics as well as check diagnostics #16210
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
Conversation
Report unreachable code in JS files when --checkjs is passed, but not otherwise.
@amcasey, |
// For JavaScript files, we don't want to report semantic errors unless explicitly requested. | ||
const includeCheckDiagnostics = !isSourceFileJavaScript(sourceFile) || isCheckJsEnabledForFile(sourceFile, options); | ||
const checkDiagnostics = includeCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : []; | ||
const includeBindAndCheckDiagnostics = !isSourceFileJavaScript(sourceFile) || isCheckJsEnabledForFile(sourceFile, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emptyArray
instead of []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. What's the difference? Fewer allocations?
src/compiler/program.ts
Outdated
const checkDiagnostics = includeCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : []; | ||
const includeBindAndCheckDiagnostics = !isSourceFileJavaScript(sourceFile) || isCheckJsEnabledForFile(sourceFile, options); | ||
const bindDiagnostics = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : []; | ||
const checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here too
also you need to accept baselines |
@mhegazy, rather than updating the baselines, would it make more sense to add set checkjs in the newly failing tests? It looks like they wanted the errors. |
sounds good. |
@mhegazy, a couple of the baselines are showing some new bind errors. Can you please confirm that they're acceptable? |
The baselines match the behaviour of .ts files, so the changes are fine. Although the strict mode messages are basically dupes of the normal messages, which is not great and should maybe be fixed separately. |
I'll start porting this to 2.3 now. |
For example, do not report unreachable code in JS files unless --checkjs is passed.