image-builder-frontend vs image-builder type definition strictness (optional fields, etc...)

[lucasgarfield]

Discussion to follow shortly...

[regexowl]

This topic came to my mind when looking at the way we define types in wizardSlice. It's a mix of existing types and artificially tightened types with the only difference usually being whether they allow undefined value or not. This creates a space for inconsistencies and potential bugs.

I believe we should settle on one way of typing values in store. It also makes sense to use types from API, but for this we would either need to tighten up the definitions or add needed checks and safeguards to the frontend code base.

The question is which approach would be better and whether anything else could be possibly influenced by the changes?

[supakeen]

This isn't tightening (narrowing) types right, it's widening them to allow T | undefined (e.g. T or undefined)?

I'm not entirely sure when the undefined clause is fulfilled here; is it when the key is not present in the hash?

[kingsleyzissou]

I guess technically the checks are stricter but the types are wider. So some context for this. I needed to explicitly add unionUndefined to the types so we could use the cockpit plugin.

See: cockpit-project/cockpit#21352 and particularly this comment here.

I enabled it in the api config with this line: https://github.com/osbuild/image-builder-frontend/blob/8ddfca0fd76b968dd9d356a652544f39d6db19e2/api/config/imageBuilder.ts#L10

The reason for this is that we were setting some values in the code to undefined
I'll try give an example to clarify:

type Foo = {
  id: string;
  description?: string;
}

// no bueno
const bar = {
  id: 'whatever',
  description: undefined,
}

// ok
const bar = {
  id: 'whatever',
}

Or we could just define the type as:

type Foo = {
  id: string;
  description?: string | undefined;
}

// ok
const bar = {
  id: 'whatever',
  description: undefined,
}