moleculer icon indicating copy to clipboard operation
moleculer copied to clipboard

Fixes typesciprt-errors with typescript version 4.8.2

Open SockeThe2nd opened this issue 3 years ago • 3 comments

:memo: Description

Fixes typescript-errors when using v4.8.2 of typescript

:dart: Relevant issues

https://github.com/moleculerjs/moleculer/issues/1129

:gem: Type of change

  • [X] 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 not work as expected)
  • [ ] This change requires a documentation update

:scroll: Example code

None needed. Current ts-tests npm run test:ts fail when using v4.8 of typescript

:vertical_traffic_light: How Has This Been Tested?

Updated the typescript-version in the project to 4.8.2, then ran the ts-test npm run test:ts

:checkered_flag: Checklist:

  • [X] My code follows the style guidelines of this project
  • [X] 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
    • Old tests failed with current version of ts
  • [X] New and existing unit tests pass locally with my changes
  • [X] I have commented my code, particularly in hard-to-understand areas

SockeThe2nd avatar Aug 29 '22 12:08 SockeThe2nd

I thought I had commented on this the other day, but I'm not sure what happened.

While I agree with the change (i.e. all of these locations should extend ServiceSettingSchema) this would represent a breaking change for any consumers that do not currently have a settings type that does extend ServiceSettingSchema. Those consumers could be using a type which does not without causing any problem provided they did not attempt to access any of the built-in properties on settings. So, this change as is I believe would represent a breaking change for existing users and would need to be deferred until a new release.

In the meantime, I am wondering if instead of this breaking change if changing:

class Service<S = ServiceSettingSchema> implements ServiceSchema {

to

class Service<S = ServiceSettingSchema> implements ServiceSchema<S> {

would bypass the TS errors and also avoid the breaking change. If that change works, it could be merged immediately to resolve the TS errors and then allow this proposed change to be deferred until a breaking change release was ready to go.

shawnmcknight avatar Sep 02 '22 18:09 shawnmcknight

I implemented the requested change for the Service class. I am unsure if further changes to the internal types/interfaces I touched are needed.

SockeThe2nd avatar Sep 05 '22 10:09 SockeThe2nd

@shawnmcknight could you approve it, plz?

icebob avatar Sep 06 '22 16:09 icebob