DOMStringMap can lead to easy type errors

[Pinpickle]

TypeScript Version: 2.1.5 (bug is present on master)

Code

const element = document.createElement('div');

const dataset = element.dataset;
dataset['key1'] = 'Hello';

console.log(dataset['key1'].toUpperCase()); // HELLO
console.log(dataset['key2'].toUpperCase()); // Cannot read property 'toUpperCase' of undefined

Expected behavior:

The project doesn't compile with a type error.

Actual behavior:

The project does compile, and a runtime error occurs.

This is caused by the fact that DOMStringMap's index return has type string, and not string | undefined.

We want

interface DOMStringMap {
    [name: string]: string;
}

To be

interface DOMStringMap {
    [name: string]: string | undefined;
}

If this is approved, I am very happy to make a PR on this - and would like to do so to try and contribute to TypeScript codebase.

[mhegazy]

This proposal will make the two lines flagged as errors now.. but i suppose it is safer.

[Pinpickle]

That's a good point - I'd rather that happen. I will open a PR tomorrow.

Is this a breaking change?

[mhegazy]

it is a breaking change. marking it as such.

[Pinpickle]

After doing some more research, it seems as though I need to modify https://github.com/Microsoft/TSJS-lib-generator/blob/master/inputfiles/browser.webidl.xml

According to the readme of that repo, these are generated by Edge.

Does this make the issue a non-starter?

[mhegazy]

you need to override https://github.com/Microsoft/TSJS-lib-generator/blob/master/inputfiles/overridingTypes.json instead. not the .xml file.

[HolgerJeromin]

Every object map can be undefined on a random key access. Can we catch this in a general approach or do we have to find every needed definition?

[Pinpickle]

@HolgerJeromin that's a good point. For numbers when access is array-like, including undefined doesn't make much sense but for strings it does. Does anyone else have opinions on this?

[HolgerJeromin]

I think even with array-like access it is easy possible to get an undefined value.

I just changed my own code and had an issue with for in loops.

const StringDict: { [index: string]: string; } = {foo: 'bar'};
for (let key in StringDict) {
   const thisStringValue = StringDict[key];
}
const StringDict2: { [index: string]: string | undefined; } = {foo: 'bar'};
for (let key in StringDict2) {
   const StringValueOrUndefined = StringDict2[key];
}

I cannot imagine a case where StringValueOrUndefined is really able to become undefined. So an extra check to make TS happy would burn CPU cycles.