Stop error checking script blocks#19228
Conversation
| export function getAnyExtensionFromPath(path: string): string | undefined { | ||
| const baseFileName = getBaseFileName(path); | ||
| const extensionIndex = baseFileName.lastIndexOf("."); | ||
| if (extensionIndex >= 0) { |
There was a problem hiding this comment.
What's the file extension of something like .gitconfig? If we decide that it has no file extension (because that's not really what dot means in that position) we just have to change >= to >.
There was a problem hiding this comment.
I always think of those as files with just an extension. Not sure it matters in this case (e.g. if you have a file named .html, you probably want it treated as an HTML file.
|
|
||
| // By default, only type-check .ts, .tsx, and 'External' files (external files are added by plugins) | ||
| const includeBindAndCheckDiagnostics = sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX || | ||
| sourceFile.scriptKind === ScriptKind.External || isCheckJsEnabledForFile(sourceFile, options); |
There was a problem hiding this comment.
As an aside, I get confused every time I see a reference to "external" source files. It sounds like they relate to external projects. Maybe "plugin" or "extension" would be clearer?
There was a problem hiding this comment.
You're right, that is an aside ;-)
For this quick & urgent fix, I don't intend to do a large refactoring across both managed and script side where this terminology is used. Maybe a suggestion for later though.
| if (extraFileExtensions) { | ||
| const fileExtension = getAnyExtensionFromPath(fileName); | ||
| if (fileExtension) { | ||
| some(extraFileExtensions, info => { |
There was a problem hiding this comment.
Would it be more natural to write this with find?
| export interface JsFileExtensionInfo { | ||
| extension: string; | ||
| isMixedContent: boolean; | ||
| scriptKind?: ScriptKind; |
There was a problem hiding this comment.
I think this is populated by passing a ScriptInfo as a JsFileExtensionInfo. If that's the case, is this actually optional? Is the question mark just there in case a consumer has created their own implementation of the interface (i.e. for back compat)?
There was a problem hiding this comment.
I misunderstood - it's passed by the client.
There was a problem hiding this comment.
For back-compat reasons for this late fix, I've tried to make the changes as benign as possible, rather than requiring extra fields. I may look for areas when porting to master where changes can be more explicit in changing expectations.
|
|
||
| // By default, only type-check .ts, .tsx, and 'External' files (external files are added by plugins) | ||
| const includeBindAndCheckDiagnostics = sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX || | ||
| sourceFile.scriptKind === ScriptKind.External || isCheckJsEnabledForFile(sourceFile, options); |
There was a problem hiding this comment.
Does something about our parsing prevent checkJsDirective from being set for non-JS files (e.g. for JSON files)?
There was a problem hiding this comment.
Wouldn't that fall through to the isCheckJsEnabledForFile(sourceFile, options) at the end of the OR clauses if so?
amcasey
left a comment
There was a problem hiding this comment.
LGTM. My only actual concern is that we might need to check for a JS script kind before/while calling isCheckJsEnabledForFile.
Set the script kind as provided by the editor for the extra file extensions in the 'configure' command.
Without this, the ScriptKind was being left undefined, and later
ensureScriptKindwas defaulting it to TypeScript - which means it was getting type checked by default.CC @mhegazy @amcasey