Convert most of core.ts to accept ReadonlyArray #17092
Conversation
ghost
requested a review
from 
src/compiler/core.ts
Outdated
| } | ||
| 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>.
|
|
||
| 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.
| * 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.
| * 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.
2 participants
For example, we can now
mapover readonly arrays.