search

microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.65k stars 12.44k forks source link

nullability of getAttribute / setAttribute APIs in lib.d.ts #9350

Closed rkirov closed 8 years ago

rkirov commented 8 years ago

TypeScript Version:

nightly (1.9.0-dev.20160217)

Code The following code does not typecheck

el1.setAttribute('foo', el2.getAttribute('foo'));

because getAttribute returns string | null, and setAttribute expects string. If would be convenient if lib.d.ts has a matching signature on the get/set pair, to avoid users having to write an explicit null check.

The two options are both types being string | null or both being string. I can see arguments for either.

According to https://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-666EE0F9 correctly getAttribute returns empty string for missing attrs, which is a strong argument for string.

The spec does not talk about nullabilty in setAttribute, but my tests a few browsers show that also null is an accepted value by the runtime. This would be an argument in favor of string | null.

aluanhaddad commented 8 years ago

I think it needs to be the former since getAttribute will return null if there is no matching attribute.

rkirov commented 8 years ago

Indeed, the spec says

The Attr value as a string, or the empty string if that attribute does not have a specified or default value.

but of course looks like all browsers just return null.

aluanhaddad commented 8 years ago

Interesting. It looks like the spec needs to be brought in line with reality.

mhegazy commented 8 years ago

PRs would be appreciated. i would make make them both accept null. seems conservative but still convenient.

mhegazy commented 8 years ago

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

YuichiNukiyama commented 8 years ago

According to MDN, setAttribute method accept null. But, this isn't remove attribute. whereas this will coerce the null value to the string "null". Isn't this confused developers?

weswigham commented 8 years ago

@YuichiNukiyama It's allowed ionly since it falls out from the string coercion rules as '' + null === 'null'. I agree that not including it in the signature is likely correct - passing null is likely an error, even if it technically is acceptable.

If you do do the construct in the OP: el1.setAttribute('foo', el2.getAttribute('foo')); and the value is missing you end up replacing the nothing that was the attribute value with the string "null" - which seems like a really subtle error.

rkirov commented 8 years ago

pretty good argument for keeping it as is. Setting attributes to 'null' is probably not what developers want.