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

Add more strict types and better inference #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kalin8900
Copy link

@Kalin8900 Kalin8900 commented May 18, 2023

Hello everyone.

I would like to propose an extension to the current solution to introduce better type inference and type safety.

Please let me know what you think. If you find it useful, I will update the documentation.

@Kalin8900 Kalin8900 requested a review from engelmi as a code owner May 18, 2023 13:13
@Kalin8900 Kalin8900 changed the title feat: add more strict types and better inference Add more strict types and better inference May 18, 2023
@engelmi
Copy link
Owner

engelmi commented May 22, 2023

Hi @Kalin8900, thank you for your contribution! I am currently a bit swamped with work, but I'll definitely have look at it the next days.

@engelmi
Copy link
Owner

engelmi commented Jun 19, 2023

Hi @Kalin8900,
what do you want to achieve with the type-safety and where do you want to have it?
In general, I like the idea of using generics to pin down the types. But at the same time the API gets quite a bit more complex, I think. And, although this seems feasible by using a decorator, when using a more general Pointcut, e.g. .*, it is not applicable.

So I am wondering if it is sufficient for your use case to have the "type-safety" on the AspectContext and not type-check if the method the aspect is applied on has the required parameter/return/error types.
So what we could do is sth like this:

// make the Aspects context generic with the default AspectContext 
export interface Aspect<T extends AspectContext = AspectContext> {
    execute(ctx: T): any;
}

export interface AspectContext {
    target: any;
    methodName: string;
    functionParams: any[];
    returnValue: any;
    error: any;
};

// ...

// specify a custom context with a number as return type
interface SpecificAspectContext extends AspectContext {
    target: any;
    methodName: string;
    functionParams: any[];
    returnValue: number;
    error: any;
}
class SpecificAspect implements Aspect {
    execute(ctx: SpecificAspectContext) {
        executeHasBeenCalled = true;
        ctx.returnValue = 2;  // works
        ctx.returnValue = "2"; // error
    }
}
class YetAnotherSampleClass {
    @UseAspect(Advice.After, SpecificAspect)
    public doIt(): number {
        return 1;
    }
}

This way you can pretend to work with specific types in the Aspects execute, but since we don't evaluate it on the decorator, it might be off and the user/developer needs to be careful still. What do you think?

@Kalin8900
Copy link
Author

Hello @engelmi!

The main thing I would like to achieve is having the type check on the decorator level. I don't see a point of adding it only on the Aspect level, because you can have unexpected behaviors then. Maybe there is a way to combine the decorator level check if specifying custom context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants