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 type checking to resolve object #1592

Merged

Conversation

acerbisgianluca
Copy link
Contributor

@acerbisgianluca acerbisgianluca commented Sep 16, 2023

closes #1591

Proposed Changes

Add type checking to this.$delegate(...) resolve object and to this.$resolve(...).

If EVENTS type isn't passed to the extended BaseComponent, Jovo should behave as usual.

If EVENTS type is set, then IDE will provide autocompletion for resolve keys and for the event name in this.$resolve(eventName).

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

}

export interface DelegateOptions<
CONFIG extends UnknownObject | undefined = UnknownObject | undefined,
EVENTS extends string = string,
> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
resolve: Record<EVENTS, string | ((this: BaseComponent, ...args: any[]) => any)>;
resolve: Partial<Record<EVENTS, string | ((this: BaseComponent, ...args: any[]) => any)>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolve object now accepts undefined values in order to not require all events (due to how Record<EVENTS, ...> behave).

@acerbisgianluca acerbisgianluca changed the title ✨ Add type checking for resolve object ✨ Add type checking to resolve object Sep 16, 2023
@acerbisgianluca
Copy link
Contributor Author

In the previous commit I've reverted the changes made to JovoComponentInfo in favor of using the infer keyword to infer the correct type from the BaseComponent's generics.
As a result, I had to override the $resolve function in BaseComponent class in order to have access to the EVENTS generic type.
In the case of $delegate I didn't need to do that, because we pass the component class to the function, so TS is able to automatically infer the correct type.
I believe we can use the infer keyword to infer the CONFIG type as well, but I've left as is. Let me know if I should change that.

@aswetlow
Copy link
Member

Hey @acerbisgianluca

Thank you for the PR. This is really useful. Do you have a working example?

@acerbisgianluca
Copy link
Contributor Author

@aswetlow Sure. You can try it in framework/test/e2e.test.ts (these lines) with your IDE by using the following code:

@Component({ global: true })
    class GlobalComponent extends BaseComponent {
      [BuiltInHandler.Launch]() {
        return this.$delegate(DelegateTargetComponent, {
          resolve: {
            onNo: this.onFinishDelegate,
          },
        });
      }

      onFinishDelegate() {
        return this.$send('Finish');
      }
    }

    @Component()
    class DelegateTargetComponent extends BaseComponent<{}, {}, 'onYes' | 'onNo'> {
      [BuiltInHandler.Start]() {
        return this.$send('Hello world');
      }

      async [BuiltInHandler.Unhandled]() {
        return this.$resolve('onYes');
      }
    }

Copy link
Member

@jankoenig jankoenig left a comment

Choose a reason for hiding this comment

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

Thanks a lot @acerbisgianluca!

@jankoenig jankoenig merged commit a2ec227 into jovotech:v4/dev Jan 31, 2024
6 checks passed
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.

None yet

3 participants