Skip to content

Discriminated union(?) not working when fields have same name? #37343

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

Closed
Svish opened this issue Mar 11, 2020 · 3 comments
Closed

Discriminated union(?) not working when fields have same name? #37343

Svish opened this issue Mar 11, 2020 · 3 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Svish
Copy link

Svish commented Mar 11, 2020

TypeScript Version:
3.8.3

Search Terms:
I haven't really searched because I don't know what words to use for this. 😕

Expected behavior:
No Typescript errors or warnings when calling makeTable.

Actual behavior:

Following Typescript errors and warnings:

  • rows gets type string[], rather than Data[], for some reason
  • row in the render function ends up as implicitly any regardless.

Typescript gets everything right if I rename one of the render properties to something else, but shouldn't a discriminated union(?) work regardless of what name I use?

Plan was to use a if( 'field' in column ) type-guard, which would then tell me which parameters to call render with, but Typescript started complaining before I even got there. 😕

Code

type Data = { id: number, name: string };
const data: Data[] = [
  { id: 1, name: 'Alice' },
  { id: 2, name: 'Bob' }
]

type Header = { header: string };

type ValueFromField<Row> = {
  [Key in keyof Row]: {
    field: Key;
    render?: (value: Row[Key]) => string;
  };
}[keyof Row];

type ValueFromRow<Row> = {
  render: (row: Row) => string;
};

type Column<Row> = Header & (ValueFromField<Row> | ValueFromRow<Row>);

const makeTable = <Row>(rows: Row[], columns: Column<Row>[]) => {
  // Pretend we're rendering a table here...
}

makeTable(data, [
  { header: 'Id', field: 'id' },
  { header: 'Name', field: 'name', render: value => value.toUpperCase() },
  { header: 'Both', render: row => `${row.id} : ${row.name}` },
])

makeTable(data, [
  { header: 'Id', field: 'id' },
  { header: 'Name', field: 'name', render: value => value.toUpperCase() },
])

makeTable(data, [
  { header: 'Test', render: row => `${row.id} : ${row.name}` },
])
Output
"use strict";
const data = [
    { id: 1, name: 'Alice' },
    { id: 2, name: 'Bob' }
];
const makeTable = (rows, columns) => {
    // Pretend we're rendering a table here...
};
makeTable(data, [
    { header: 'Id', field: 'id' },
    { header: 'Name', field: 'name', render: value => value.toUpperCase() },
    { header: 'Both', render: row => `${row.id} : ${row.name}` },
]);
makeTable(data, [
    { header: 'Id', field: 'id' },
    { header: 'Name', field: 'name', render: value => value.toUpperCase() },
]);
makeTable(data, [
    { header: 'Test', render: row => `${row.id} : ${row.name}` },
]);
Compiler Options
{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "declaration": true,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext"
  }
}

Playground Link: Provided

@RyanCavanaugh
Copy link
Member

This isn't a discriminated union because there's no discriminant.

This pattern isn't very easy to model because it's susceptible to aliasing effects because you're counting on the absence of the field property to define behavior, but object types in TS aren't closed, so we don't have a way to guarantee that the field isn't present.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Mar 11, 2020
@Svish
Copy link
Author

Svish commented Mar 11, 2020

This isn't a discriminated union because there's no discriminant.

Aaah, so that's what makes it a discriminated union. I figured it was was just using | with objects. Thanks for clearing that up.

This pattern isn't very easy to model because it's susceptible to aliasing effects because you're counting on the absence of the field property to define behavior, but object types in TS aren't closed, so we don't have a way to guarantee that the field isn't present.

So if I understand you correctly, to use discriminating unions, there has to be a common property that is always present and accountable?

What do you mean with "guarantee that the field isn't present"? In my example, the intention seems clear to me, and in my newb mind typescript would force me into using the right combination? But is the issue when objects are coming "from the outside", so typescript doesn't have control over them?

@RyanCavanaugh
Copy link
Member

For example, this is legal code (according to TS) that will certainly cause a crash in the presence of an actual implementation of makeTable:

const obj = {
  header: "Won't work",
  render: (arg: Data) => arg.name.toLowerCase(),
  field: "id"
};
const objRef: Header & ValueFromRow<Data> = obj;
makeTable(data, [objRef]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

2 participants