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

Infer method name for parameter decorator #30102

Open
SalathielGenese opened this issue Feb 26, 2019 · 7 comments
Open

Infer method name for parameter decorator #30102

SalathielGenese opened this issue Feb 26, 2019 · 7 comments
Labels
Needs Investigation This issue needs a team member to investigate its status.
Milestone

Comments

@SalathielGenese
Copy link

TypeScript Version: 3.3.3
Tried with @next (3.4.0-dev.201xxxxx) ? NO

Search Terms: type:issues infer method name parameter decorator

Code

declare function Method():
{
    (t: any, m: 'method'): void;
}

declare function Parameter():
{
    (t: any, m: 'method', i: number): void;
}

class Play
{
    @Method() // err
    public another_method ( @Parameter() /* NO ERROR */ test: number ): void
    {
        throw new Error('Not yet implemented');
    }
    @Method()
    public method ( @Parameter() test: number ): void
    {
        throw new Error('Not yet implemented');
    }
}

Expected behavior: @Parameter() should error just as @Method() when applied to another_method() because the m parameter is typed "method" (which should reduced the set of method). i.e method name should be enforced.

Actual behavior: No error

Playground Link: https://typescript-play.js.org

Use case :

I crafted a type that filters method names with the nth parameters iff that parameters is of the given type T.

See KeysToTypedNthParameter definition here

let hash = {
    a( _0: string, _1: string ) {},
    b( _0: string, _1: number ) {},
    c( _0: number, _1: number ) {},
    d( _0: number, _1: string ) {},
}

let string_0: KeysToTypedNthParameter<0, string, typeof hash>; // "a" | "b"
let string_1: KeysToTypedNthParameter<1, string, typeof hash>; // "a" | "d"
let number_0: KeysToTypedNthParameter<0, number, typeof hash>; // "c" | "d"
let number_1: KeysToTypedNthParameter<1, number, typeof hash>; // "c" | "d"

Later, I used that type to infer method name for my parameter decorator - @Inject - in the excerpt below, I marked the ONLY line that should error and why (but both lines actually error)

class Test
{
    public blatantly!: boolean;
}

class Play
{
    public method(
        @Inject({ type: Test })
        @Inject({ type: Number }) // err (construtor mismatch instance type)
        test: Test,
    ): void
    {
        throw new Error( 'Not yet implemented' );
    }
}
Play;

Here is @Inject decorator definition and complete code -

const Inject: InjectLike = void 0 as unknown as InjectLike;

interface InjectLike
{
    <
        C extends ConstructorLike,
    >
    ({}: { type: C }): {
        <
            // FIXME : not enough to infer the method name - see ./__playground.ts#
            M extends ( T extends ConstructorLike ? never : KeysToTypedNthParameter<I, InstanceType<C>, T> ),
            I extends number,
            T,
        >
        ( target: T, member: M, index: I ): void;
    };
}
@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Feb 26, 2019
@SalathielGenese
Copy link
Author

Hi everyone,

Can I do anything to quicken this thread ? I've discussed the same day with @dragomirtitian on Gitter and we are will to PR to patch this.

Thanks.

@dragomirtitian
Copy link
Contributor

@rbuckton @RyanCavanaugh

Just as an experiment I made the changes we (me an @SalathielGenese) think are required, you can see them here

These changes do the following:

  1. For parameterIndex the number literal type is used instead of number
  2. For propertyKey the literal type is used instead or undefined if the decorator is used on a constructor parameter
  3. For target the actual type of the target is passed in, like it is for method decorators.

Uses cases enabled:

  1. We can restrict a parameter decorator based on the target (we can restrict the decorator to only be applied on a constructor, or a instance method or a static method)
  2. We can restrict a parameter decorator based on the type of the parameter type.
type FilterKeysByMethodArg<T, TIndex extends number, TParam> = {
    [P in keyof T] : T[P] extends (...p: infer A) => any ? 
        A extends Record<TIndex, TParam> ? P: never: never
}[keyof T]
declare function TypedParameter<TParam>():
{
    <T extends Record<TName, (...p: any[]) => any>, TIndex extends number, TName extends FilterKeysByMethodArg<T, TIndex, TParam>>(t: T, m: TName, pIndex: TIndex): void
}
class Play
{
    public method (
        @TypedParameter<string>() o: number, //  err
        @TypedParameter<number>() test: number // ok 
    ): void
    {
        throw new Error('Not yet implemented');
    }
}

Problems:

  1. Since propertyKey can be typed undefined the lib version of ParameterDecorator is not really correct anymore, propertyKey should be string | symbol | undefined
  2. Some existing decorators may raise compile time errors now when used on constructor parameters (since propertyKey will be typed as undefined in this case). I would argue this is correct behavior, since undefined was always a possible value for the propertyKey parameter.

@SalathielGenese
Copy link
Author

SalathielGenese commented Mar 6, 2019

Thanks @dragomirtitian . I hope we'll hear from them, now that a PR illustrates the issue we're trying to solve. Maybe the label Needs Investigation will be less appropriate ?

Anyhow, much thanks !!!

@Veetaha
Copy link

Veetaha commented Mar 13, 2019

Stumbled with such a problem recently. I think it is definitely not that hard to fix it. Let's make TypeScript strongly typed again!

@dragomirtitian
Copy link
Contributor

Hi any movement on this? Now that the 3.4 release rush is over can I submit a PR with the code I have for this (@DanielRosenwasser , @RyanCavanaugh, @rbuckton) ?

@dgreene1
Copy link

dgreene1 commented Sep 3, 2019

We would love to see this opened up for @dragomirtitian’s PR so that we could take advantage of it in tsoa which is one of the more popular libraries for runtime validation and generation of Swagger YAML from TypeScript types. And currently we would have the potential issue:

function postSomething( @Body<Thing1>() aParameterWhosTypeIsNotInferred: Thing2 ): Thing2

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Sep 3, 2019

@dgreene1 Just speculating, but probably since decorators are such a mess (with regard to spec compatibility, since the proposal changed) in TS right now you will not see any changes to decorators any time soon. The current decorator proposal is still stage 2 and actually does not include parameter decorators which will be in a future version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants