Convert most of core.ts to accept ReadonlyArray #17092
Conversation
src/compiler/core.ts
Outdated
| @@ -429,7 +433,7 @@ namespace ts { | |||
| addRange(result, v); | |||
| } | |||
| else { | |||
| result.push(v); | |||
| result.push(v as T); | |||
There was a problem hiding this comment.
Is this cast here because the isArray helper doesn't guard for readonly array types? Can it not be overloaded to do so?
There was a problem hiding this comment.
For some reason I thought changing isArray would break things. But it seems to work fine to return x is ReadonlyArray<any>.
| @@ -2010,7 +2010,7 @@ namespace ts { | |||
| } | |||
|
|
|||
| function getAccessibleSymbolChainFromSymbolTableWorker(symbols: SymbolTable, visitedSymbolTables: SymbolTable[]): Symbol[] { | |||
| if (contains(visitedSymbolTables, symbols)) { | |||
| if (contains<SymbolTable>(visitedSymbolTables, symbols)) { | |||
There was a problem hiding this comment.
Why does this need an explicit type argument now?
There was a problem hiding this comment.
There are problems with inferring type arguments -- when you get Foo[] and expect ReadonlyArray<T>, we have trouble inferring that T = Foo.
| @@ -661,6 +669,8 @@ namespace ts { | |||
| /** | |||
| * Compacts an array, removing any falsey elements. | |||
| */ | |||
| export function compact<T>(array: T[]): T[]; | |||
| export function compact<T>(array: ReadonlyArray<T>): ReadonlyArray<T>; | |||
There was a problem hiding this comment.
Is this overload order correct? Won't we just always pick the top one, since a ReadonlyArray will be compatible with it (so we'll never go down the list)? (Same comment on singleOrMany, concatenate, sameFlatMap, sameMap, and filter)
There was a problem hiding this comment.
The more-specific overload should go first -- T[] is more specific than ReadonlyArray since it contains everything ReadonlyArray has, plus mutators.
| @@ -164,7 +164,7 @@ namespace ts { | |||
| * LS host can optionally implement these methods to support completions for module specifiers. | |||
| * Without these methods, only completions for ambient modules will be provided. | |||
| */ | |||
| readDirectory?(path: string, extensions?: string[], exclude?: string[], include?: string[], depth?: number): string[]; | |||
| readDirectory?(path: string, extensions?: ReadonlyArray<string>, exclude?: ReadonlyArray<string>, include?: ReadonlyArray<string>, depth?: number): string[]; | |||
There was a problem hiding this comment.
Is it ok to change this? Does this count as a breaking change?
There was a problem hiding this comment.
Technically, but code should never have modified the extensions array anyway.
For example, we can now
mapover readonly arrays.