Open OliverJAsh opened 3 years ago
Arguably, it should be possible to use the ?
optional suffix on an index signature to force noUncheckedIndexedAccess
for that index signature, e.g.:
interface Dict<T> {
[key: string]?: T;
}
declare const foo: Dict<number>;
// $ExpectType number | undefined
foo.prop;
// ^?
if (foo.prop != null) {
// $ExpectType number
foo.prop;
// ^?
}
I think what's needed is a way to represent a Dict<V>
(or Record<string, V>
) type that is partial over string
keys, rather than complete. Adding the ?
optional suffix to the indexer type currently causes typescript to infer that the dictionary may contain undefined
values, not just that some keys may be absent.
Lots of libraries / APIs in javascript use an Object with string keys that are not statically known, and values of a fixed non-null type. Some examples:
process.env
, defined as Dict<string>
in process.d.ts
APIGatewayProxyEventBase.queryStringParameters
in aws-lambda
, defined as Record<string, string|undefined>
in api-gateway-proxy.d.ts
in @types/aws-lambda
These cannot be satisfactorily represented in Typescript at present, because there is no way to have both of the following desirable properties at the same time:
r[k]
is of type V | undefined
Object.values(r)
is of type V[]
There are currently two choices that library authors can take to represent this:
A: Dict<V>
or Record<string, V>
or interface Dict<V> { [key: string]: V; }
-- this satisfies (2) but not (1).
This approach incorrectly assumes that all string lookups in process.env
will return a string, but most will return undefined
at runtime.
B: Dict<V | undefined>
or Record<string, V | undefined>
or interface Dict2<V> { [key: string]?: V; }
-- this satisfies (1) but not (2).
This approach incorrectly assumes that iterating over the object may return undefined
entries, i.e. Object.keys(event.queryStringParameters)
is of type (string | undefined)[]
rather than string[]
because the library authors have chosen approach B to avoid the drawbacks of A.
Unless I'm missing something, there is no way to declare a type in Typescript that represents a js object with both properties (1) and (2).
I think this is needed in addition to the noUncheckedIndexedAccess
flag, because library authors (like @types/aws-lambda
) want to be able to avoid the pitfalls of A
but can't do so without adding the drawbacks of B
.
To clarify my above comment, which is perhaps over-long, sorry:
I think Typescript needs to add a new built in type that allows library authors to fully represent js objects used as dictionaries, because the current language support falls short (can't have both "1" and "2" as described above).
The recent changes to add noUncheckedIndexedAccess
were a step in the right direction towards fixing this issue (which has been known about for a long time) but they are insufficient for library authors in the Typescript ecosystem, since it requires user opt-in. In the absence of a full fix for this issue, some library authors have been adding "| undefined
" to their declarations, which fixes use-case (1) above but breaks use-case (2).
I've found that noUncheckedIndexedAccess
really goes too far with the addition of undefined
.
Code that checks for property array index should not have to deal with undefined
but does and encounters errors. For example:
interface Rec { [prop: string]: string };
function foo(rec: Rec, key: keyof Rec): string {
if (key in rec) return rec[key]; // error: TS(2322) Type 'string | undefined' is not assignable to type 'string'
rec[key] = 'placeholder'; // ${key} now exists in rec
return rec[key]; // error: TS(2322) Type 'string | undefined' is not assignable to type 'string'
}
function bar(): string {
const strings = ["bar"];
if (strings.length > 0) return strings[0]; // error: TS(2322) Type 'string | undefined' is not assignable to type 'string'
return 'huh';
}
playground So instead of enabling in it we've added notes of caution in code that we must remember to check that properties exist.
@jason-ha I think this is my big issue with noUncheckedIndexedAccess. I would love to enable it, but it forces undefined on both Array and Record accesses alike. Quite a number of closed issues discuss this problem, and the concensus is that TypeScript simply can't know if an Array access is undefined or not and so it treats any indexed access as possibly undefined.
But this creates so many false positives for the programmer, as simply going through a for(let i = 0; i < foo.length; i++)
loop causes false positives for any foo[i]
access.
I would propose that noUncheckedIndexedAccess be the default for TypeScript, but only for Records, and not Arrays. Then we could have another option, maybe noUncheckedArrayAccess that also makes Array access possibly undefined for those that want that feature.
That change would solve this issue, whereby existing type definitions don't need to go adding undefined to all of their Record types, since it would be added by default to their indexed access in TypeScript. And additionally we wouldn't get all of the array access headaches associated with enabling noUncheckedIndexedAccess.
lib Update Request
Before we had
noUncheckedIndexedAccess
, it was common practice to addundefined
to an index signature as a workaround. For example:DOMStringMap
(lib.dom.d.ts
) was modified to includeundefined
.@types/node
were modified to includeundefined
(e.g.ProcessEnv
andParsedUrlQuery
).However, it seems this workaround wasn't extended to all index signatures, e.g.
RegExpMatchArray['groups']
inlib.es2018.regexp.d.ts
andPropertyDescriptorMap
inlib.es5.d.ts
.Now we have
noUncheckedIndexedAccess
I would like to ask what the best way forward is regarding these workarounds? For people who enablenoUncheckedIndexedAccess
, these workarounds are redundant. For people who don't enablenoUncheckedIndexedAccess
, these workarounds are still necessary—but potentially not always desirable since it forces a higher level of strictness.It seems inconsistent that some index signatures use this workaround and others do not, so I think we should decide either to use this workaround in all index signatures or remove it from all index signatures and suggest people use
noUncheckedIndexedAccess
.Including
undefined
creates a few problems, for example:If the TypeScript team can decide the best way forward then this will also provide some guidance to the community—for example we're trying to decide what the best option is for
@types/node
: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/51177.