Return `String` should be nullable, no?

[kevmoo]

web/web/lib/src/dom/html.dart

Line 1281 in 6538aca

external String operator [](String name);

CC @srujzs

[srujzs]

This is similar to #181. The IDL claims the return type is non-nullable, but naturally it would be nice to coerce missing values to null instead of a cast failure. I'm not sure if the behavior is similar everywhere a getter is declared in the IDL to make that change. The current solution is using has to check for existence.

[rutvik110]

I'm not sure if the behavior is similar everywhere a getter is declared in the IDL to make that change. The current solution is using has to check for existence.

From what I observed in the idl specs, the behaviour isn't consistent. Some getters are non-nullable while some are defined nullable. I don't know yet what determines that though.

But it would be nice to make this nullable as the users won't have to remember to check for the nullable case by using has, it would be explicit by the returned value from the getter. I could contribute to this issue if you could share what changes we might be looking towards fixing this issue beside just making this getter nullable.

[srujzs]

I don't know yet what determines that though.

I think it's just a matter of whether the value is allowed to be null or not in most cases. IME, if a getter cast fails due to a null exception, it's usually because the browser/context doesn't set that field, but the IDL dictates it should be, which then requires feature detection.

An example of why you might still want to use has instead of the getter you're proposing is if the key exists, but the value is null. The current interface doesn't let you set that through Dart, but it's definitely possible with DOMStringMap. It wouldn't be clear what this getter should return in such a case (returning null may mistakenly imply non-existence). I'm also generally against special-casing logic in the translator for specific APIs unless it breaks otherwise.

I do recognize there's friction with having to use has with package:web though beyond just this interface, I'm just not sure what the ideal fix is without requiring a lot of null-checks everywhere.

[isoos]

This is a super surprising bug that prevents easy migration. From the API I'd expect it to be returning empty value or throw an Exception, but instead we are getting null when the value is missing. If this is an edge case, it may not be covered in tests and will cause a production issue later down the line.

Please fix this, or at minimum put up big warning signs in the documentation.

[srujzs]

From the API I'd expect it to be returning empty value or throw an Exception, but instead we are getting null when the value is missing.

Sorry - for clarification, you should be seeing an exception in Dart when the value is missing. Specifically, you should see a cast from null to String exception. Are you seeing something else or are you saying the JS API itself doesn't throw an exception (which is true, it should return undefined)?

Please fix this, or at minimum put up big warning signs in the documentation.

I'm okay adding a warning here for this getter that the code will throw if the value is missing or null.

[isoos]

Specifically, you should see a cast from null to String exception.

Yes, and my first intuitive solution was to cast it to String? - which did work. I was tempted to leave as-is, however, I did dig deeper until I've found this issue about it, and then started fixing it with the has call before using the value getter. But I am sure this is not the right developer experience, and I think the API should be true to the real type of the returned value: String?.