Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NamedNodeMap/getNamedItem is not defined to be potentially "null" #17695

Closed
DorianGrey opened this issue Aug 9, 2017 · 5 comments
Closed

NamedNodeMap/getNamedItem is not defined to be potentially "null" #17695

DorianGrey opened this issue Aug 9, 2017 · 5 comments
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this

Comments

@DorianGrey
Copy link

TypeScript Version: 2.4.2

I've recently faced in issue accessing the NamedNodeMap of a Node - a user managed to insert an img tag without a src attribute. My code accessed attributes.getNamedItem("src") on the node, and (accidentially) did not check if the resulting attr exists before accessing its .value, and the function crashed with a type error.
While not performing this check is my personal mistake, I've figured out that getNamedItem is not defined to possibly return null: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.es6.d.ts#L14075

However, this function will return null in case the attributes does not exist:
https://developer.mozilla.org/en-US/docs/Web/API/NamedNodeMap/getNamedItem
This should hold true for getNamedItemNS as well.

Thus, as I've strict resp. strictNullChecks enabled in in my tsconfig.json, the compiler should raise in error when not checking the result for null (see code below).

Code

const node: Node = // ... an <img> node
const src: Attr = this.attributes.getNamedItem("src"); // should complain that src should be of type Attr | null
const valueLength = src.value.length; // should complain that src might be null.

Expected behavior:
The code mentioned above compiles without an error that src might be null, even with the config strict resp. strictNullChecks enabled. It does because the definition mentions that the result of getNamedItem is always an Attr.

Actual behavior:
The code mentioned above should raise a compiler error in case strict resp. strictNullChecks is enabled. The definition should mention that the result of getNamedItem is of type Attr | null.

@mhegazy mhegazy added Help Wanted You can do this Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Aug 22, 2017
@mhegazy mhegazy added this to the Community milestone Aug 22, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

PRs welcomed. You can find more information about contributing lib.d.ts fixes at https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md#contributing-libdts-fixes.

@dsifford
Copy link
Contributor

@mhegazy I went in to try and fix this issue now and I'm having trouble understanding where exactly you want these fixes to take place?

Reading the contributing guide, it appears that all the DOM-related types come from https://github.com/Microsoft/TSJS-lib-generator, however, when I went into that repo to see where it was I could make the fix for this, the only location where there is any mention of NamedNodeMap is also in auto-generated files.

Any help pointing me to the correct file to edit would be greatly appreciated.

Thanks in advance.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 14, 2017

The change should go into https://github.com/Microsoft/TSJS-lib-generator/blob/master/inputfiles/overridingTypes.json, see https://github.com/Microsoft/TSJS-lib-generator#contribution-guidelines for more details.

@NN---
Copy link

NN--- commented Mar 20, 2018

Is it possible to tell TypeScript that null check is not required in this code ?

for (let i = 0; i < nodeMap.length; ++i) {
   const node = nodeMap.item(i);
   // node is never null since i is less than nodeMap.length , but check is required.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 20, 2018

@NN--- const node = nodeMap.item(i)!;

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fixed A PR has been merged for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants