timocov / ts-transformer-properties-rename

TypeScript custom transformer to rename properties
MIT License
70 stars 3 forks source link

Handle object literals with "erased" type #19

Closed mgrebenets closed 3 years ago

mgrebenets commented 3 years ago

Bug report

I believe it's one of the optimisations TypeScript compiler is doing, like type interning, but the type of the object literal can be "erased" down to an anonymous type when used in combination with generic function.

The examples below give more context.

The Model interface is annotated as @public and should not be minified. However, when the generic context function gets in the middle, the type checker ends up "erasing" the type of the object literal being returned, i.e. instead of type being Model it becomes an anonymous { param1: string, param2: string }. As a result the minifier will now minify property names it should not, breaking the code.

Input code

/** @public */
interface Model {
    param1: string;
    param2: string;
}

export declare function context<Result>(name: string, producer: () => Result): Result;

export function exampleWithGeneric(param1: string, param2: string): Model {
    return context("contextName", () => {
        return {
            param1: param1,
            param2: param2,
        };
    });
}

export function exampleWithGenericAndAnnotation(param1: string, param2: string): Model {
    return context<Model>("contextName", () => {
        return {
            param1: param1,
            param2: param2,
        };
    });
}

export function exampleWithoutGeneric(param1: string, param2: string): Model {
    return {
        param1: param1,
        param2: param2,
    };
}

Expected output


/** @public */
interface Model {
    param1: string;
    param2: string;
}

export declare function context<Result>(name: string, producer: () => Result): Result;

export function exampleWithGeneric(param1: string, param2: string): Model {
    return context("contextName", () => {
        return {
            param1: param1,
            param2: param2,
        };
    });
}

export function exampleWithGenericAndAnnotation(param1: string, param2: string): Model {
    return context<Model>("contextName", () => {
        return {
            param1: param1,
            param2: param2,
        };
    });
}

export function exampleWithoutGeneric(param1: string, param2: string): Model {
    return {
        param1: param1,
        param2: param2,
    };
}

Actual output

/** @public */
interface Model {
    param1: string;
    param2: string;
}

export declare function context<Result>(name: string, producer: () => Result): Result;

export function exampleWithGeneric(param1: string, param2: string): Model {
    return context("contextName", () => {
        return {
            _internal_param1: param1,
            _internal_param2: param2,
        };
    });
}

export function exampleWithGenericAndAnnotation(param1: string, param2: string): Model {
    return context<Model>("contextName", () => {
        return {
            param1: param1,
            param2: param2,
        };
    });
}

export function exampleWithoutGeneric(param1: string, param2: string): Model {
    return {
        param1: param1,
        param2: param2,
    };
}

Additional context

It's worth noting that it seems to be how TypeScript compiler resolves types. Even VS Code's LSP indicates it.

image image

Annotating generic functions with type signature is an option, but not a very reliable one because I don't think there's a way to enforce it. If there's such option with linter or compiler that'd be nice.

It feels like transformer plugin has to do the work compiler chose not to. E.g. if object literal is part of return statement then the plugin would have to analyse return type of the parent function and then all the other parent functions all the way up to the top (source file) and check if any of those types is external or annotated with @public.

timocov commented 3 years ago

Hi there,

It's worth noting that it seems to be how TypeScript compiler resolves types.

Yes, as far as we have a duck typing TypeScript compiler will match types in later stages, but it won't assign a name Model (in your case) automatically since you didn't specify it anywhere. You can test it by doing some interesting trick: just move cursor to the first param1 in param1: param1 in return statement object and press "navigate to the symbol" (f12 in vscode) - it won't go anywhere.

If there's such option with linter or compiler that'd be nice.

I think it's possible to write such eslint rule for sure, but most probably it will require a type checking information. Also, I think this case is suitable for https://github.com/timocov/ts-transformer-properties-rename/issues/10, but the transformer can't generate errors/warnings properly at the moment I'm afraid.

For my experience of maintaining lightweight-charts (we use this transformer there for a long time) I can tell that if you have any kind of test (manual or automated ones) it's much much easier to maintain the project to have it work with this transformer once you setup it and check everything known at the moment of introducing.

It feels like transformer plugin has to do the work compiler chose not to.

Oh no, I don't think that this transformer should do this work in any way πŸ™‚ I mean, Microsoft has the whole team to support compiler and type checking especially, I don't think that part of compiler's work should be done somewhere outside, just because it will be outdated on every new version of the compiler. Just look at any of JetBrains IDE with their own TypeScript implementation - they have to do a lot of work every new TypeScript release just because they don't use TypeScript's compiler and they still have a lot of unimplemented features so for (for instance they don't have support for composite projects what makes work on big project painful).

This is might be huge amount of work actually, I think it's much easier to implement eslint rule to enforce people to write types for everything πŸ˜‚ (probably tslint/eslint already have such rule but I don't remember exactly).

We have had exactly the same issue when we introduced this transformer in lightweight-charts some time ago and fixed it by adding a generic specifier, e.g. https://github.com/tradingview/lightweight-charts/commit/080aa9aa52a10499f1c038db970b3f9cab1c6332#diff-8bbee54ab053b73e1b695d299bc1f31998a043bd96da02dc2d4020e9169e2d97R153 - for me it's the best solution of this issue.