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: Update default type for ServiceSchema generic to Service<S>. #1299

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marceliwac
Copy link
Contributor

📝 Description

Fix for the default type for ServiceSchema generic which was previously set to void, causing problems when This type was not provided. The type should have defaulted to Service<S>.

🎯 Relevant issues

Fixes #1297.

Original PR which introduced the problem #1272.

💎 Type of change

  • Bug fix (non-breaking change which fixes an issue)

📜 Example code

Given this example setup:

interface GreeterSettings extends ServiceSettingSchema {
        defaultName: string;
}

interface GreeterMethods {
        uppercase(str: string): string;
        other(): void;
}

interface GreeterLocalVars {
        myVar: string;
}

type GreeterThis = Service<GreeterSettings> & GreeterMethods & GreeterLocalVars;

This used to be causing the problems for the following cases:

// Without providing types for ServiceSchema at all
const GreeterService: ServiceSchema = {
        // ...
}

// Without providing `T` generic, default of `void` for "This" type caused problems.
const GreeterService: ServiceSchema<GreeterSettings> = {
        // ...
}

While the following was the only working use-case:

// Explicitly providing T generic worked as expected
const GreeterService: ServiceSchema<GreeterSettings, GreeterThis> = {
        // ...
}

Applying this patch fixes the problem for following cases:

// Without providing types for ServiceSchema at all
const GreeterService: ServiceSchema = {
        // ...
}

// Without providing `T` generic, default of `Service<S>` allows for the code to work as expected.
const GreeterService: ServiceSchema<GreeterSettings> = {
        // ...
}

// Explicitly providing T generic worked as expected
const GreeterService: ServiceSchema<GreeterSettings, GreeterThis> = {
        // ...
}

🚦 How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Tested locally against TypeScript compiler (version 4.9.4).
  • Tested locally against tsd (version 0.22.0).
  • Added unit tests that verify the types set for lifecycle handlers in different scenarios (when schema provides setting type, schema provides setting type and this type, schema provides no type).

🏁 Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

Copy link

pkg-pr-new bot commented Sep 14, 2024

Open in Stackblitz

npm i https://pkg.pr.new/moleculerjs/moleculer@1299

commit: efb777c

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.

New type for ServiceSchema breaks ServiceBroker.createService and interface Options
1 participant