Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timeseries: fix setting reset causing weird behavior #5458

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

stephanwlee
Copy link
Contributor

PR #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.

// 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,

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.

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.
@stephanwlee stephanwlee merged commit 8f54f42 into tensorflow:master Dec 14, 2021
@stephanwlee stephanwlee deleted the bug branch December 14, 2021 16:10
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
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.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants