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

Regression in type inference from 2.5 -> 2.6 #19668

Closed
phiresky opened this issue Nov 1, 2017 · 7 comments
Closed

Regression in type inference from 2.5 -> 2.6 #19668

phiresky opened this issue Nov 1, 2017 · 7 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@phiresky
Copy link

phiresky commented Nov 1, 2017

TypeScript Version: 2.6.0-rc, 2.7.0-dev.20171101

Code Runnable Gist

import * as s from "@hediet/typed-sql";
import { Pool } from "pg";

const db = new s.DbConnection(new s.PostgreQueryService(new Pool()));

const customers = s.table(
    "customers",
    { name: s.tText, country: s.tText },
    { id: s.tInteger }
);

const orders = s.table(
    "orders",
    { customerId: customers.id.type, orderDate: s.tText },
    { id: s.tInteger }
);

async function test() {
    return await db.exec(
        s
            .from(customers)
            .where({ country: "US" })
            .leftJoin(orders)
            .on({ id: orders.id })
            .select(orders.$all)
    );
}
{
    "compilerOptions": {
        "target": "es6",
        "module": "commonjs",
        "strict": true
    }
}

Expected behavior:

Correctly inferred type of test() (Typescript 2.5.3):

function test(): Promise<{
    customerId: number;
    orderDate: string;
    id: number;
}[]>

Actual behavior:

2.6.0-rc:

function test(): Promise<void>

2.7.0-dev.20171029:

function test(): Promise<{
    [x: string]: any;
}[]>

No type errors are shown in either version.

@ahejlsberg
Copy link
Member

ahejlsberg commented Nov 1, 2017

I'm getting lots of errors when I compile the example (after installing the referenced npm modules). In particular, many of the errors relate to the fix implemented in #17922. It appears the @hediet/typed-sql package attempts to access type parameters in a base class expression. This never worked correctly and is now an error. See #17829 (comment) for how to implement this pattern correctly.

@ahejlsberg ahejlsberg added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 1, 2017
@phiresky
Copy link
Author

phiresky commented Nov 2, 2017

Right, compiling with tsc does indeed show type errors in the library, in VSCode it does not show them. @hediet, can you look into it?

@zsparal
Copy link

zsparal commented Nov 2, 2017

Is there an easy way to properly type generic classes that extend from a class expression? The use-case is Immutable.js's Record class:

Common:

interface NodeProps<N> {
  value: N;
  edges: List<string>;
}

Here's what I want to do:

export class Node<N> extends Record<NodeProps<N>>(defaultNode) {
  setValue(value: N): Node<N> {
    return this.set("value", value as never);
  }
}

Unfortunately this gives me Node.value: any so here's what I came up with as a workaround:

interface NodeMethods<N> {
  setValue(value: N): Node<N>;
}

class NodeBase<N> extends Record<NodeProps<any>>(defaultNode) implements NodeMethods<N> {
  setValue(value: N): Node<N> {
    return this.set("value", value);
  }
}

export type Node<N> = Readonly<NodeProps<N>> & NodeMethods<N> & Record<NodeProps<N>>;
export function Node<N>(values: Partial<NodeProps<N>>): Node<N> {
  return new NodeBase(values);
}

Is there a better way to go about this?

Edit: Sorry if this belongs to #17829

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 4, 2017

Record<NodeProps<N>>(defaultNode) does not make sense since Record requires 2 type parameters, a class cannot extend a type, and a type cannot be called. Are you trying to write a mixin?

@zsparal
Copy link

zsparal commented Nov 5, 2017

I'm using version 4.0.0-rc.9 of immutable which only requires a single type parameter (TProps) for defining a Record. The two relevant type definitions:

export function Factory<TProps extends Object>(values?: Partial<TProps> | Iterable<[string, any]>): Record<TProps> & Readonly<TProps>;
export function Record<TProps>(defaultValues: TProps, name?: string): Record.Factory<TProps>;

Extending from Record is documented in the docs and it works correctly except when using a generic interface as TProps . This used to work incorrectly, using any instead of the proper type parameter before 2.6 and fails with a compile-time error after 2.6.

Here's a minimal repro that (unlike Record) happens to work correctly with 2.5 but still fails with 2.6:

export function MyRecord<T>() {
  return class {
    value: T;
  };
}

export class Foo<T> extends MyRecord<T>() {}

@aluanhaddad
Copy link
Contributor

Ah, I see, I though you were referencing TypeScript's built in Record<K, T> mapped type. Since as mentioned was never correct, you can either write

export function MyRecord<T = any>() {
  return class {
    value: T;
  };
}

export class Foo<T> extends MyRecord() {}

to obtain an approximation of the old behavior.

Or write

export function MyRecord() {
  return class<T> {
    value: T;
  };
}

export class Foo<T> extends MyRecord()<T> {}
const foo = new Foo<string>();
foo.value.toLocaleLowerCase();

to pass the type parameter to the expression that results from calling the class factory.

@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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants