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.

@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, @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...