Incomplete Video types

[jozefchutka]

separating issues in #1556

existing VideoEncoderConfig to be modified (adding undefined):

interface VideoEncoderConfig {
    alpha?: AlphaOption | undefined;
    avc?: AvcEncoderConfig | undefined;
    bitrate?: number | undefined;
    bitrateMode?: VideoEncoderBitrateMode | undefined;
    codec: string;
    displayHeight?: number | undefined;
    displayWidth?: number | undefined;
    framerate?: number | undefined;
    hardwareAcceleration?: HardwareAcceleration | undefined;
    height: number;
    latencyMode?: LatencyMode | undefined;
    scalabilityMode?: string | undefined;
    width: number;
}

same for VideoFrameBufferInit:

interface VideoFrameBufferInit {
    codedHeight: number;
    codedWidth: number;
    colorSpace?: VideoColorSpaceInit | undefined;
    displayHeight?: number | undefined;
    displayWidth?: number | undefined;
    duration?: number | undefined;
    format: VideoPixelFormat;
    layout?: PlaneLayout[] | undefined;
    timestamp: number;
    visibleRect?: DOMRectInit | undefined;
}

and VideoDecoderConfig:

interface VideoDecoderConfig {
    codec: string;
    codedHeight?: number | undefined;
    codedWidth?: number | undefined;
    colorSpace?: VideoColorSpaceInit | undefined;
    description?: BufferSource | undefined;
    displayAspectHeight?: number | undefined;
    displayAspectWidth?: number | undefined;
    hardwareAcceleration?: HardwareAcceleration | undefined;
    optimizeForLatency?: boolean | undefined;
}

while its unexpected a VideoFrame to have format=null https://developer.mozilla.org/en-US/docs/Web/API/VideoFrame/format

interface VideoFrame {
...
    readonly format: VideoPixelFormat;
...
}

[saschanaz]

I don't think explicit undefined is needed for optional properties?

[jozefchutka]

Here is how I need to create config following current types:

const config:VideoEncoderConfig = {codec, width, height};
if(alpha !== undefined)
    config.alpha = alpha;
if(avc !== undefined)
    config.avc = avc;
if(bitrate !== undefined)
    config.bitrate = bitrate;
if(bitrateMode !== undefined)
    config.bitrateMode = bitrateMode;
// ... some more if()-s to follow here
return config;

Here is how it can be created with having undefined in place:

const config:VideoEncoderConfig = {codec, width, height, alpha, avc, bitrate, bitrateMode ...}

The reason is exactOptionalPropertyTypes

Please have a look at these types https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/dom-webcodecs/webcodecs.generated.d.ts#L136 ... I used to have it in place before switched to official TypeScript-DOM-lib-generator

[saschanaz]

I mean | undefined is implied by using ? and thus not needed. Your example already works with the current lib.

[jozefchutka]

Here I have created a demonstration for you

[saschanaz]

Oh, TIL exactOptionalPropertyTypes. I'm not sure the point to add | undefined and thus effectively disable exactOptionalPropertyTypes which you explicitly enabled, though?

[saschanaz]

Per the documentation: https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes

However, this isn’t quite accurate colorThemeOverride: undefined is not the same as colorThemeOverride not being defined. For example "colorThemeOverride" in settings would have different behavior with undefined as the key compared to not being defined.

This is not true for any web APIs and thus I don't think we should add | undefined. undefined is very consciously made to behave as nonexistent value in Web IDL.

[jozefchutka]

Considering this lib is the official one to use with typescript, it should work with the TS strict setup.
Please consider the great impact and thousands of projects relying on these types, would need to either rewrite the code or loose restrictions.

There are good reasons for exactOptionalPropertyTypes as described in documentation.

undefiend vs. not defined is obviously not an issue for web APIs runtime. As you just learned about exactOptionalPropertyTypes today while you worked with {foo:undefined} daily.

[saschanaz]

Oh, hmm, maybe I interpreted the purpose of this flag backward. It's still for code that does not treat undefined as same as the nonexistent value, but other codes that do treat them as same can have explicit | undefined to suppress the error.

Nathan Shively-Sanders (@sandersn), am I correct here and should we add | undefined for all optional properties here?

[sandersn]

Hm. Well, if a property behaves the same whether you explicitly assign undefined or leave it unassigned, then it should be declared a p?: X | undefined instead of p?: X. I think this is what you mean when you say

undefined is very consciously made to behave as nonexistent value in Web IDL.

But maybe you meant that assigning undefined behaves differently than not assigning at all?

Unfortunately, if the two behave the same, that means we need to add | undefined to every optional property. I did this in a few very large PRs on Definitely Typed, but I forgot about the DOM types.

[saschanaz]

But maybe you meant that assigning undefined behaves differently than not assigning at all?

It does not behave differently, it should behave same or it'll be considered as a design error.

So I guess the answer is to go add | undefined!

[saschanaz]

Given there's a PR now, Jake Bailey (@jakebailey) do you also agree with #1557 (comment) ? I figured it's time to double check. We probably would get type compat issue with DT...

[jakebailey]

The problem is that DT will not be running with exactOptionalPropertyTypes outside of a few things (Node I think has it on now), so we largely won't be able to check how breaky this is or not. Without that option, this is effectively a noop.

This is a change I've been wondering about for a while, but I am fairly worried about doing it at the moment given the PR seems to affect every single optional property? We're trying to get 6.0 settled so we can get to 7.0 and so on, so I don't really want to create an even larger blast radius with these types...

I can try and apply this patch to the main repo and run things to see, but I do think this is pretty bad timing.

[jakebailey]

It's also the case that the main lib.d.ts doesn't have this in many places, and I don't personally know the history to know if that's on purpose or not.

[jakebailey]

I do suspect that this will break cases where the types overlap; as things get added here and conflict to preexisting shims, we "fix" them on DT by duplicating and migrating to the new definition from this repo. But, this I think would basically apply to everything, which is why I get a bad vibe.