type: enable exactOptionalPropertyTypes option
#5459
Conversation
PR tensorflow#5030 divided up the default setting from overrides and, in selector, we have mixed two values in selectors to make a concrete settings value. When reset, overriden values were set to `undefined` and caused the settings to be very wrong. ```ts // This resolves to {a: undefined} {...{a: 1}, ...{a: undefined}} ``` Object spread (or `assign`) does not skip a property whose value is `undefined` and causes settingOverride with undefined value to take precedence over the default value which, in the end, resolves the image brightness/contrast values to `undefined` unlike what the types indicated. In the end, this is a fault of the type system since the interface is defined as `Partial<>` and a value should really not have `undefined` value. To illustrate, ```ts interface Foo { bar: number; } // Legal; `bar` property may not exist const foo2: Partial<Foo> = { }; // Legal; `bar` is defined as `number` but here, it is `undefined` // yet it is legal. const foo3: Partial<Foo> = { bar: undefined }; ``` TypeScript 4.4 has a way to property guard against `undefined` value in a `Partial<>` type, `"exactOptionalPropertyTypes"` but TensorBoard app currently violates the type definition so we will enable the tsconfig in a subsequent changes.
Before this change, below held true:
```ts
interface Foo {
bar?: number;
}
// Legal
const foo1: Foo = {bar: undefined};
// Legal
const foo2: Foo = {bar: undefined};
```
After this change,
```ts
interface Foo {
bar?: number;
baz: number|undefined;
}
// Illegal; `bar` cannot be `undefined` and must be number.
const foo1: Foo = {bar: undefined, baz: undefined};
// Illegal; `baz` property must exist. Must be one of `number` of
// `undefined`.
const foo1: Foo = {};
// Legal; `bar` property does not exist while `baz` is specified.
const foo2: Foo = {baz: undefined};
```
When a property is defined as `?:`, it does not mean that the property
can be `undefined` but it means a property can be omitted altogether or
it must exist with the value of the type as defined. As such, when a
property is written with `undefined`, the value type must explicitly
allow `undefined` like `baz` example above.
| @@ -441,7 +441,8 @@ export class RouteConfigs { | |||
| deepLinkProvider: definition.deepLinkProvider ?? null, | |||
| pathname: definition.path, | |||
| params: {}, | |||
| originateFromRedirection: wasRedirected, | |||
| originateFromRedirection: true, | |||
There was a problem hiding this comment.
It's not clear to me if this is an intentional change in behavior -- originateFromRedirection was "wasRedirected" but is now always "true".
There was a problem hiding this comment.
This was an intentional change since this is handling the case where no route definition matched current url and we are "redirecting" to the default route. Also, hard coding originateFromRedirection: true keeps type very simple and clear.
| const partialRequest: TimeSeriesRequest = isSingleRunPlugin(plugin) | ||
| ? {plugin, tag, runId: runId!} | ||
| : {plugin, tag, experimentIds}; | ||
| if (sample !== undefined) { |
There was a problem hiding this comment.
It seems you have two major choices for fixing errors arising from this new compiler option:
- Convert property from
property?: typetoproperty: type | undefined - Keep
property?: typebut guard setting the property only if we know there is a value to set it to -- for example:if (valueToSet !== undefined) property.type = valueToSet
It seems like you applied these options roughly equally in this change. Did you have some methodology to deciding whether to use option 1 or option 2?
There was a problem hiding this comment.
Binary classification roughly happened like this:
- is this an internal type where we can be more explicit about existence of a property? If so, I preferred
<T | undefined>overprop?: Tsince conditionally forming a data structure that conditionally has a property is more difficult and is harder to validate. For instance,foo.hasOwnProperty('propName')forces"propName"to be understood by the compiler and mangle accordingly or becomes incompatible with mangler like Closure compiler. - if server responses/requests can omit a property, requires a property to be omitted, or is more efficient with a property removed than having undefined value for each one of them, I preferred
<T | undefined>.
Metrics data source does not do much transformation to convert a backend type to an internal type so this module, specifically, has a lot of the mixed pattern.
| @@ -84,7 +84,7 @@ export class TBHttpClient implements TBHttpClientInterface { | |||
| path: string, | |||
| // Angular's HttpClient is typed exactly this way. | |||
| body: any | null, | |||
| options: PostOptions = {} | |||
| options: PostOptions | undefined = {} | |||
There was a problem hiding this comment.
What does it mean for this to have both undefined and a default value of {}? Does the default value mean it's still an optional property? But there are still some cases where you want to explicitly set it to undefined?
There was a problem hiding this comment.
Unfortunately, TypeScript does not seem to understand that options cannot be undefined when having default value like this. undefined was introduce TypeScript compiler and that's all.
No, there isn't a case where we would like options to ever be undefined but we still guard against that for now.
Understood but it looks like the task was scrapped not because it is wrong but because (1) it is difficult to do LSC and (2) unclear whether developers truly intended to use |
Before this change, below held true:
```ts
interface Foo {
bar?: number;
}
// Legal
const foo1: Foo = {};
// Legal
const foo2: Foo = {bar: undefined};
```
After this change,
```ts
interface Foo {
bar?: number;
baz: number|undefined;
}
// Illegal; `bar` cannot be `undefined` and must be number.
const foo1: Foo = {bar: undefined, baz: undefined};
// Illegal; `baz` property must exist. Must be one of `number` of
// `undefined`.
const foo2: Foo = {};
// Legal; `bar` property does not exist while `baz` is specified.
const foo3: Foo = {baz: undefined};
```
When a property is defined as `?:`, it does not mean that the property
can be `undefined` but it means a property can be omitted altogether or
it must exist with the value of the type as defined. As such, when a
property is written with `undefined`, the value type must explicitly
allow `undefined` like `baz` example above.
Before this change, below held true:
```ts
interface Foo {
bar?: number;
}
// Legal
const foo1: Foo = {};
// Legal
const foo2: Foo = {bar: undefined};
```
After this change,
```ts
interface Foo {
bar?: number;
baz: number|undefined;
}
// Illegal; `bar` cannot be `undefined` and must be number.
const foo1: Foo = {bar: undefined, baz: undefined};
// Illegal; `baz` property must exist. Must be one of `number` of
// `undefined`.
const foo2: Foo = {};
// Legal; `bar` property does not exist while `baz` is specified.
const foo3: Foo = {baz: undefined};
```
When a property is defined as `?:`, it does not mean that the property
can be `undefined` but it means a property can be omitted altogether or
it must exist with the value of the type as defined. As such, when a
property is written with `undefined`, the value type must explicitly
allow `undefined` like `baz` example above.
Before this change, below held true:
After this change,
When a property is defined as
?:, it does not mean that the propertycan be
undefinedbut it means a property can be omitted altogether orit must exist with the value of the type as defined. As such, when a
property is written with
undefined, the value type must explicitlyallow
undefinedlikebazexample above.Note: this change currently includes #5458. Please review the second commit and beyond.