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

fix(idempotency): preserve scope of decorated class #2693

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

This PR addresses the bug in the linked issue that caused class methods decorated with the @idempotent decorator to lose the original scope of the class.

In simpler terms, when working with classes it's common to reference other class methods or properties. In JavaScript this is done via the this keyword, which in the case of classes is bound to the class instance.

For example, in the code snippet below the handler method is able to access the logger property by using the this keyword, which at runtime is bound to the fooInstance instance.

export class Foo {
    private readonly logger = console;

    public async handler(event: unknown): Promise<string> {
        this.logger.debug('Received event');

        return 'success';
    }
}

const fooInstance = new Foo();

When working with class method decorators in TypeScript it's important to keep in mind that the decorator replaces the method implementation with one it defines. In doing so, it's important to pass down the original scope so that the decorated method maintains access to the this scope.

This PR makes changes to the makeIdempotent function, the IdempotencyHandler class, and the idempotent decorator implementation so that this happens.

Specifically, the PR:

  • converts the makeIdempotent function from an arrow function to a regular function so that it maintains a binding to this
  • modifies the idempotent decorator to explicitly .bind(this) on the makeIdempotent function when replacing the implementation of the decorated method
  • updates the IdempotencyHandler to now accept a new thisArg parameter and restore it onto the original function when calling it

I have also added a new unit test case and modified one of the integration test cases to confirm that the fix works.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: closes #2692


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Jun 26, 2024
@dreamorosi dreamorosi requested review from a team as code owners June 26, 2024 12:28
@boring-cyborg boring-cyborg bot added idempotency This item relates to the Idempotency Utility tests PRs that add or change tests labels Jun 26, 2024
@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label Jun 26, 2024
Copy link

@github-actions github-actions bot added the bug Something isn't working label Jun 26, 2024
@dreamorosi
Copy link
Contributor Author

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch and a quick PR with the fix, Thank you Andrea!

Thanks @HaaLeo for reporting the issue!

@am29d am29d merged commit 22ec1f0 into main Jun 26, 2024
55 checks passed
@am29d am29d deleted the fix/idempotency_class_scope branch June 26, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working idempotency This item relates to the Idempotency Utility size/M PR between 30-99 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: idempotent decorator in combination does not maintain class scope
2 participants