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

[Feature request] Support for type checking properties in an object with different types #23927

Closed
Albert-Gao opened this issue May 6, 2018 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@Albert-Gao
Copy link

Albert-Gao commented May 6, 2018

I got an object like this:

const state = {
      isFormOK: true,
      fieldA: {
          value: 'abc';
          status: 'ok' | 'normal' | 'error';
          errorText: '';
      }
}

There could be many fields which have the same structure as the above fieldA property, and in the code, I will actually use index property to access them because they are random. So I declared types like this:

export interface FieldState {
  value: string;
  status: 'ok' | 'normal' | 'error';
  errorText: string;
}

export interface ComponentState {
  isFormOK: boolean;
  [fieldName: string]: FieldState
}

If you just swap those interface with type, then it works for flow.js. Whereas can't do that in Typescript, because as the doc said:

While string index signatures are a powerful way to describe the “dictionary” pattern, they also enforce that all properties match their return type. This is because a string index declares that obj.property is also available as obj["property"].

And the error is:

Property isFormOK of type boolean is not assignable to string index type FieldState

I think it's fine, because when I access the state using an index, I might go to the case where the index equals isFormOK but I still want to use it like a FieldState. I know. But how could I solve it and type check this object?

Currently, we have 2 options:

  1. Tried changing [fieldName: string]: FieldState to [fieldName: string]: {}, then cast it to FieldState everytime when use it. it works. But then the type for state doesn't convey the message where the real structure of this object.

  2. Tried changing ComponentState to

export type ComponentState =
    { isFormOK: boolean } &
    { [fieldName: string]: FieldState }

I currently use the 2nd approach.

What about just something like:

export interface ComponentState {
  isFormOK: boolean;
  [fieldName: string]: FieldState
}

Then the [fieldName: string] will exclude isFormOK when type checking the related instance.

Because we need some syntax to describe the actural structure of an object, so maybe reusing the existing syntax but expanding its underneath check will support it easily. Just like flow.js.

@Albert-Gao Albert-Gao changed the title Support for properties in an object with different types [Feature request] Support for properties in an object with different types May 6, 2018
@jack-williams
Copy link
Collaborator

Possibly related: #22509

@Albert-Gao Albert-Gao changed the title [Feature request] Support for properties in an object with different types [Feature request] Support for type checking properties in an object with different types May 7, 2018
@Albert-Gao
Copy link
Author

Thanks, and another similar one too, #17867

@Albert-Gao
Copy link
Author

Albert-Gao commented May 7, 2018

Which means, currently, there is no optimal way to describe an object as the one I mentioned in the post like I did in flow.js with a type definition like the following?

type ComponentState {
  isFormOK: boolean;
  [fieldName: string]: FieldState
}

@MartinJohns
Copy link
Contributor

Removing the isFormOK from the possible indexer types will introduce hard-to-track type-errors in your code, if you ever have a fieldName that has the same name. You have a string with the run-time value isFormOK, you use the indexer and you assume the value is a FieldState, when it's actually not. The type-checker can't help here at all.

Honestly, the cleanest approach is just to adjust your data structure to the needs you have:

export interface ComponentState {
  isFormOK: boolean;
  fields: { [fieldName: string]: FieldState };
}

Now you represent your data structure type-safe and clean. You don't mix the actual fields with your status information anymore.

@jack-williams
Copy link
Collaborator

jack-williams commented May 7, 2018

Flow might let you write that, but it's clearly unsound for computed properties.

If you were to support that in TS, then computed properties should probably be the intersection for set, so boolean & FieldState, and the union for get, so boolean | FieldState.

const state: ComponentState;
let prop: string;
state[prop] = ... // must be of type boolean & FieldState
const result: boolean | FieldState = state[prop] // we get the union back

Although this brings in usability issues because there is no way to describe a string that is not isFormOk, so updating an object of type ComponentState without using a cast is hard in the general case.

const state: ComponentState;
let prop: string;
if(prop === "isFormOk") {
  state[prop] = true // ok as we've narrowed prop
} else {
  state[prop] = ...;
  // prop is still string here, without the knowledge 
  // that it cannot be isFormOk. We are forced to use
  // the intersection (and consequently a cast).
}

@mhegazy mhegazy added the Duplicate An existing issue was already created label May 7, 2018
@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants