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(semver): Allow unsetting build metadata #3157

Merged

Conversation

justinmchase
Copy link
Contributor

@justinmchase justinmchase commented Feb 1, 2023

The main issue is if you pass "" as the build metadata to the semver.increment function it doesn't actually unset the build metadata as expected because of a falsy comparison of the parameter rather than a strict metadata === undefined.

This will now alter the behavior of the increment function to have no changes when you specify undefined but to actually set the build metadata to [] if you were to pass "" into the function.

Additionally, there was what I think to be a "bug" discovered in the constructor which was making it very hard to test, which is a bug where the this.raw field was set before the call to this.format(), which ends up retaining a wrong / different version than if you call increment.

Namely if you create a semver with build metadata the build metadata will be in the raw field. If you then increment it, then it will fall out of the raw field even if the build metadata is unchanged.

This happens because in the increment function, it is calling this.format() and then setting the raw field to the version but in the constructor it does the reverse. This means the raw will change based on if you've called increment or not.

Also, the whole design of this class probably needs a major revamp to be immutable. Having mutations happen on functions such as .format() is really problematic when the object is this complex. If the std lib ever hits 1.0.0 I would strongly recommend putting this chore onto the backlog.

Before

const v = semver.parse("1.2.3+0");
v.increment("patch", undefined, "");
console.log(v.format({ style: "full" })); // 1.2.4+0

After

const v = semver.parse("1.2.3+0");
v.increment("patch", undefined, "");
console.log(v.format({ style: "full" })); // 1.2.4
const v = semver.parse("1.2.3+4");
v.increment("patch");
console.log(v.format({ style: "full" })); // 1.2.4+0

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

Also, the whole design of this class probably needs a major revamp to be immutable.

This makes sense. Created an issue #3164

@kt3k kt3k merged commit 4b1bb8b into denoland:main Feb 3, 2023
@justinmchase justinmchase deleted the feature/semver-unsettable-build-metadata branch February 6, 2023 19:04
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

2 participants