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

TS2345 error with discriminated unions #17480

Closed
webartoli opened this issue Jul 28, 2017 · 11 comments
Closed

TS2345 error with discriminated unions #17480

webartoli opened this issue Jul 28, 2017 · 11 comments
Labels
Duplicate An existing issue was already created

Comments

@webartoli
Copy link

TypeScript Version: 2.4.2

Code

interface AddTodoAction {
  type: 'ADD_TODO'
  id: number
  text: string
}

interface ToggleTodoAction {
  type: 'TOGGLE_TODO'
  id: number
}

type TodosAction = AddTodoAction | ToggleTodoAction

const action = {
  type: 'TOGGLE_TODO',
  id: 1
}

const target: TodosAction = action
      ^^^^^^

// or

const fun = (a: TodosAction) => {

}

fun(action)
    ^^^^^^

Expected behavior:

No error.

Actual behavior:

error TS2345: Argument of type '{ type: string; id: number; }' is not assignable to parameter of type 'TodosAction'.
  Type '{ type: string; id: number; }' is not assignable to type 'ToggleTodoAction'.
    Types of property 'type' are incompatible.
      Type 'string' is not assignable to type '"TOGGLE_TODO"'.
@webartoli
Copy link
Author

Additional informations

trying to use this function:

const fun = (a: TodosAction) => {

}

with action variable (const, let, var) error TS2345 appears.

const action = {
  type: 'TOGGLE_TODO',
  id: 1
}

fun(action)
    ^^^^^^

Calling function with a plain javascript object (inlining action variable) it works like expected:

fun({
  type: 'TOGGLE_TODO',
  id: 1
})

@ikatyang
Copy link
Contributor

You have to use type assertion for literal types in object, otherwise TS will widen them into their super-type, e.g. string, number, etc.

const action1 = {
  type: 'TOGGLE_TODO',
  id: 1
}; //=> { type: string, id: number }
const action2 = {
  type: 'TOGGLE_TODO' as 'TOGGLE_TODO',
  id: 1
}; //=> { type: 'TOGGLE_TODO', id: number }
const action3 = {
  type: 'TOGGLE_TODO',
  id: 1 as 1
}; //=> { type: string, id: 1 }
const action4 = {
  type: 'TOGGLE_TODO' as 'TOGGLE_TODO',
  id: 1 as 1
}; //=> { type: 'TOGGLE_TODO', id: 1 }

@webartoli
Copy link
Author

webartoli commented Jul 28, 2017

Of course, but in some cases TS can handle properly type inference with no type assertion for literal types needed

fun({
  type: 'TOGGLE_TODO',
  id: 1
}) // => works

probabily I'm missing something: but I was expecting to do type assertion for literals in any cases (or - even better - in no case)

@MicahZoltu
Copy link
Contributor

Hmm, is there no other way to work around this? The source of the input is JSON, which means I don't have control over it so I can't do the as 'FOO' everywhere. I really want to strongly type my function though and use literal union types in the object definition.

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Jul 28, 2017

My issue (and the one mentioned here) can be worked around by being explicit about the type on original assignment:

const action: TodosAction = {
  type: 'TOGGLE_TODO',
  id: 1
}

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 28, 2017

I still feel like a workaround that would be correct and intuitive would be to write

const action1 = {
  get type() { return 'TOGGLE_TODO'; },
  id: 1
};

since it creates a value with an immutable type property, inferring the literal type makes sense.

I proposed this in #11467 but I believe it was either declined or there was little interest in it.

It is also, in my view, inconsistent since if you specify readonly on a class property, a literal type will be inferred even though the property is not truly readonly, whereas if get is used, it is actually readonly.

@MicahZoltu FYI, you can use

```ts

to make you code blocks highlight correctly.

@gcnew
Copy link
Contributor

gcnew commented Jul 28, 2017

Maybe I'm asking the obvious, but why not annotate action?

const action: TodosAction = {
  type: 'TOGGLE_TODO',
  id: 1
};

Literal types are not inferred, unless TS knows it's your intention to do so. This can be achieved either by type assertions, type annotations or other forms of context.

@webartoli
Copy link
Author

In my code actually I'm annotatin action and is very verbose.

I know that literal types are not inferred, unless TS knows it's my intention to do so, but TS infer correcly in case of inline:

fun({
  type: 'TOGGLE_TODO',
  id: 1
})

but not when a variable in involved:

const action = {
  type: 'TOGGLE_TODO',
  id: 1
}

fun(action)
    ^^^^^^

The TS error is on the last row, where the intention is as clear as the inline version; in my opinion if TS can infer type on the first case, could infer types on the second too.

@gcnew
Copy link
Contributor

gcnew commented Jul 29, 2017

When you are provide the object inline as in fun({ type: ... }) TypeScript uses the signature of fun as context (a guide not to widen types). Furthermore, this literal will not be used anywhere except for that exact invokation, which means no one will ever need to mutate its fields. In contrast, when you write const action = .., TypeScript doesn't know that your only intention is to use that value as an argument to fun. Object properties are normally mutable. Indeed, lets imagine you were not trying to make a discriminated union (TypeScript has now way to tell whether you are) then the more intuitive behaviour is to infer non-literal types for object properties, e.g.

const userComment = {
    userName: 'john_doe',
    message: 'Hello world'
};

// for security reasons hide the username
userComment.useName = userComment.useName.slice(0, 3) + '...';

Should userName be inferred as "john_doe", or message as "Hello world"? TypeScript chooses "no", both for backward compatibility reasons and because in the mutable world of JavaScript it's the more useful option.

Speaking of discriminated unions, I use constructor functions to help the compiler out and spare myself some writing:

interface ToggleTodoAction { type: 'TOGGLE_TODO', id: number }
interface AddTodoAction    { type: 'ADD_TODO',    id: number,  text: string }

type TodosAction = AddTodoAction | ToggleTodoAction

function mkToggleTodo(id: number): ToggleTodoAction {
    return { type: 'TOGGLE_TODO', id };
}

function mkAddTodo(id: number, text: string): AddTodoAction {
    return { type: 'ADD_TODO', id, text };
}

const action = mkToggleTodo(1);

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

Duplicate thread can be found at #17363 and #10416

More details about current behavior can be found in #10676

@mhegazy mhegazy added the Duplicate An existing issue was already created label Aug 22, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Sep 6, 2017

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

@mhegazy mhegazy closed this as completed Sep 6, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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

6 participants