moleculer icon indicating copy to clipboard operation
moleculer copied to clipboard

Fix version string build when an integer value is passed in string form

Open slinkardbrandon opened this issue 4 years ago • 3 comments

:memo: Description

I got tripped up on something (that was my fault) when using the broker.waitForServices() method and passing an array of objects. In my case, serviceVersion was a string even though I thought it was a number. Below is an example as to how you can see how that might have occurred. I'm just hoping this change might prevent the pain for anyone else in the future.

    await broker.waitForServices([
      { name: serviceName, version: serviceVersion },
    ]);

:dart: Relevant issues

N/A

:gem: Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

:scroll: Example code

  /**
   * This will now await for the following services to become available:
   * v1.foo & v2.bar
   * 
   * Previously this would wait for
   * v1.foo & 2.bar
   */ 
    await broker.waitForServices([
      { name: 'foo', version: 1 },
      { name: 'bar', version: '2` }
    ]);

:vertical_traffic_light: 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

  • [x] Test A - I added to the version string building static method tests with a case that demonstrated the problem, then made it work

:checkered_flag: Checklist:

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

slinkardbrandon avatar Jan 14 '21 20:01 slinkardbrandon

@slinkardbrandon thanks for the PR, but I share the viewpoint of @ngraef. Every change in this logic can cause a breaking change because users can use version as 5 or "5" but they are different in the current and all previous versions. So now I won't merge it but leave open.

icebob avatar Jan 18 '21 16:01 icebob

@icebob and @ngraef what are your thoughts on simply logging a warning statement if a version like "5" is used? That way we can preserve the existing functionality but potentially warn developers of their goof with an entry to their logger.

slinkardbrandon avatar Jan 18 '21 16:01 slinkardbrandon

@icebob and @ngraef what are your thoughts on simply logging a warning statement if a version like "5" is used? That way we can preserve the existing functionality but potentially warn developers of their goof with an entry to their logger.

you can do this by using patch-package :D

0x0a0d avatar Jul 11 '21 10:07 0x0a0d