Mark some arrays as readonly#17725
Conversation
2ed10ab to
7ca4506
Compare
weswigham
left a comment
There was a problem hiding this comment.
The change in types I'm generally OK with, but the changes in style, IMO, need justification. If they're because we're not doing things correctly, do we have issues open?
|
|
||
| if (program.getCompilerOptions().declaration) { | ||
| diagnostics = diagnostics.concat(program.getDeclarationDiagnostics(sourceFile, cancellationToken)); | ||
| diagnostics = [...diagnostics, ...program.getDeclarationDiagnostics(sourceFile, cancellationToken)]; |
There was a problem hiding this comment.
Are these changes because we don't infer types neatly between a mutable array's .concat and a ReadonlyArray? Or is it stylistic? If the former, do we have an issue open for it?
There was a problem hiding this comment.
Unfortunately, we're using three different kinds of concatenation in the codebase: ES6 ..., ES5 .concat, and ES3 ts.concatenate. The .concat() method won't typecheck against readonly arrays (#17076) but does the same thing as .... We should really prefer ts.concatenate which won't allocate output if the inputs are empty, but in this case we're currently returning a mutable Diagnostic[], so we can't use any function that might reuse its readonly inputs -- it has to be a freshly-allocated output.
| compilerResult.program ? | ||
| ts.filter(compilerResult.program.getSourceFiles(), sourceFile => !Harness.isDefaultLibraryFile(sourceFile.fileName)) : | ||
| []), | ||
| const inputSourceFiles = compilerResult.configFileSourceFiles.slice(); |
There was a problem hiding this comment.
How does this change involve ReadonlyArray? What necessitated the change in style?
There was a problem hiding this comment.
OK, this one was mostly just because I didn't want to see a closure in a filter in a ?: in a concat in a map and wanted to break it up. But also because configFileSourceFiles is now readonly and #17076 caused a compile error.
| const missing = program.getMissingFilePaths(); | ||
| assert.isDefined(missing); | ||
| assert.deepEqual(missing, ["d:/pretend/nonexistent.ts"]); // Absolute path | ||
| assert.deepEqual(missing, ["d:/pretend/nonexistent.ts" as Path]); // Absolute path |
There was a problem hiding this comment.
What makes this cast (and the similar ones below) required now, but not before? Should it have been required before?
There was a problem hiding this comment.
Yeah, I think it should have been required before. missing is a ReadonlyArray<Path>, it shouldn't be compared against a string array.
No description provided.