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: ensure SemVer instance passed to inc are not modified #427

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

louis-bompart
Copy link
Contributor

inc can be used three ways:

  1. As a an option on the CLI : npx semver 1.0.0 -i major
  2. As a method of the SemVer class: semverInstance.inc('major')
  3. As a function: inc('1.0.0', 'major')

This PR does not impact the behaviour of the first two and focuses only on the last one.

The underlying issue is described in #425, but tl;dr inc should be a pure function (I think, from its documentation on NPM), and here it has a side-effect.

I remove this side-effect by ensuring that the SemVer instance created inside the inc function is created from a string, which removes the mutation vector.

References

Depends on #426 - just to fix the tests, that's why 563d75e is polluting the changeset
Fixes #425

@lukekarrys
Copy link
Contributor

Thanks for this PR @louis-bompart! I think this is the right direction for inc as well as any other "pure" functions. Would you be willing to update this PR with this change made across all those functions? I also think we could export semver/functions/clone that would clone an instance and use that internally.

And I apologize for letting this sit so long, it now has some conficts and shouldn't need any dep updates as those have all been updated.

Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

See previous comment

@louis-bompart
Copy link
Contributor Author

I also think we could export semver/functions/clone that would clone an instance and use that internally.

That's my two cents, so take it as it is, but I think that a 'clone' function would be a bit out of place:
As a user, I'd be confused about what 'cloning' a semver is doing, especially when the other functions are quite so 'math-oriented'.

I think adding a factory function to the SemVer class would maybe be more in place.

So instead of doing:

import {clone, SemVer} from 'semver';
const verA = new SemVer('1.0.0');
const verB = clone(verA);

One would do this:

import {SemVer} from 'semver';
const verA = new SemVer('1.0.0');
const verB = SemVer.clone(verA);

Another solution would be to do nothing and use structuredClone internally. This would require to enforce Node >=17.x tho.

@louis-bompart
Copy link
Contributor Author

louis-bompart commented Apr 10, 2022

Would you be willing to update this PR with this change made across all those functions?

✔️. As far as I could tell, only parse was returning an actual SemVer object from a SemVer object.

@lukekarrys
Copy link
Contributor

Thanks @louis-bompart! This looks great!

As far as I could tell, only parse was returning an actual SemVer object from a SemVer object.

Thanks for looking into this, not sure why I thought there would be more. And sorry for the back and forth but I now think that this should only apply to inc since that is designed to change the state of a version.

I can see people using parse as a safe way to handle a variable that might be a string or a semver instance and I don't want to break the strict equality that returning the instance provides.

> s = new semver.SemVer('1.0.0')
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: '1.0.0',
  major: 1,
  minor: 0,
  patch: 0,
  prerelease: [],
  build: [],
  version: '1.0.0'
}
> s === semver.parse(s)
true
> s === semver.parse('1.0.0')
false

Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Can you remove the commit updating parse? After that I think this is good to go!

@lukekarrys lukekarrys changed the title fix: ensure SemVer instanced passed as input are not modified fix: ensure SemVer instance passed to inc are not modified Apr 11, 2022
@lukekarrys lukekarrys merged commit f070dde into npm:main Apr 11, 2022
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.

[BUG] inc affects the inputed SemVer object.
2 participants