fix(version): Resolve prerelease for version without bump

[pshrmn]

Fixes #2025

Description

This PR modifies the resolvePrereleaseId function to use the package's prerelease identifier as the default identifier when it exists, not only when when the bump CLI arg is an increment keyword.

Motivation and Context

Currently, the existing prerelease identifier is only used when lerna version is provided an increment bump. This means that if the current version is 1.0.0-beta.2, the "Custom Prerelease" prompt's default value is alpha.

How Has This Been Tested?

I ran the existing tests, but haven't added any new ones yet. I was looking at the tests in prompt-version.test.js, but those use a custom resolvePrereleaseId function. I'd be happy to add a test, but I'm not sure what the best way to test the resolvePrereleaseId function created in getVersionsForUpdates is.

I'm not actually sure why the isPrerelease check exists, so I'm not sure if removing it will break anything. I didn't have any luck tracing the code's history (it was introduced in #1522) to see why it was originally added.

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 read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[evocateur]

I would expect https://github.com/lerna/lerna/blob/master/commands/version/__tests__/version-bump-prerelease.test.js to be somewhere that this sort of TDD could be established before making changes to the command implementation.

[evocateur]

It is certainly distressing that removing this inline conditional doesn't break any tests. The prompting is very poorly tested, I'll be the first to admit.

I would expect a modification to makePromptVersion's arguments to be necessary to establish the correct preid in the dialog, at the very least. I think the best way to write the tests is to do them first, before changing this code, just so we can establish what fails in the current context. Without that, I really can't say if this is the right approach.

[pshrmn]

Sounds good. I'll revert locally and poke around with the tests to try to find the use case that relies on the isPrerelease conditional.

[pshrmn]

I've been looking at this a bit and I think that the isPrerelease condition is a no-op.

The result of resolvePrereleaseId function is the identifier passed as the third argument to semver.inc. The identifier is only used when the second argument is a pre___ type; for major, minor, and patch releases, the identifier is ignored.

If the condition is removed, a failing test would happen when isPrerelease is false and existingPreid is not undefined; the third argument to semver.inc would be existingPreid instead of alpha. However, since the value returned by resolvePrereleaseId is never used if isPrerelease is false, the value doesn't actually matter.

[evocateur]

You're right, of course. Thanks for taking the time to explain it in detail. I verified it locally with this change:

--- a/commands/version/__tests__/version-bump-prerelease.test.js
+++ b/commands/version/__tests__/version-bump-prerelease.test.js
@@ -152,8 +152,8 @@ test("independent version prerelease does not bump on every unrelated change", a
   // simulate choices for pkg-a then pkg-b
   prompt.mockChoices("patch", "PRERELEASE");
   prompt.input.mockImplementationOnce((msg, cfg) =>
-    // a convoluted way of entering "bumps" at the prompt
-    Promise.resolve(cfg.filter("bumps"))
+    // the _existing_ "bumps" prerelease ID should be preserved
+    Promise.resolve(cfg.filter())
   );
 
   await lernaVersion(cwd)();

[evocateur]

I'll update the test once this is merged. Thanks again!