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

Should I bump the major version if a change doesn't alter the public API but changes the client behaviour subtly? #819

Open
aman-agrawal-coolblue opened this issue Apr 6, 2022 · 8 comments

Comments

@aman-agrawal-coolblue
Copy link

Scenario

At my org we have a collection of internal Nuget packages that are used across teams. One such package provides Polly retries around a set of what we consider recoverable/retriable HTTP status codes. From that list, the 500 Internal Server Error code was recently removed and the minor version incremented.

The impact of upgrading this package will be that the consumers will now lose automatic retries of this status code and because the change technically doesn't break compatibility of the public APIs, chances are they won't even notice that their applications are now no longer retrying this status code. This not only means subtle change in the application behaviour but also a potential change forced on the applications that want to preserve the older behaviour (through extension points etc).

My Question

Whether or not 500 ISE should be be retried is not relevant here, let's assume that in our case it is. My question is should this change have warranted a major version increment due to the fact that it silently alters application behaviour and might even force a change to the application, or do the SemVer major version semantics only apply to changes that fail the compilation?

Let me know if more context is needed.

Cheers
Aman

@ljharb
Copy link
Contributor

ljharb commented Apr 6, 2022

Yes. The behavior is part of the API - you thus changed the API in a way that someone might have been relying on.

@aman-agrawal-coolblue
Copy link
Author

Would it be an idea to make this "edge case" clearer or more explicit in the spec? Perhaps using this scenario as an example?

@ljharb
Copy link
Contributor

ljharb commented Apr 6, 2022

I don't think it's an edge case at all.

Any change that forces the user to change their code is a breaking change - that's an incompatible change.

@jwdonahue
Copy link
Contributor

jwdonahue commented May 3, 2022

I agree with ljharb on this in principle, but since these are internal components, I wonder if someone could have checked that none of the consuming code was negatively impacted?

There's a fuzzy area around what constitutes the "documented API". If the retry code was not part of the original design and end user documentation, it can be argued that it should not have been relied upon, but that's not a terribly strong argument, especially in cases where the implementation code amounts to the only available API documentation.


If every visible behavior of an implementation were considered part of the API, the patch field would not be needed.

@jcornaz
Copy link

jcornaz commented May 3, 2022

Semver only applies to the declared API.

  • If what you change is undocumented behavior, then it is a patch (because the API has not changed).
  • If it was documented, and you only add more precision to the documentation to inform about the new behavior, then it is a minor change.
  • If it was documented, and it is necessary to change the documentation to account for the new behavior, then it is a breaking change.

@jcornaz
Copy link

jcornaz commented May 3, 2022

I don't think it's an edge case at all.

Any change that forces the user to change their code is a breaking change - that's an incompatible change.

@ljharb, It cannot be that simple. Even when you fix a bug it may force some users to change their code if they were relying on the buggy behavior without knowing it (and that kind of thing happen all the time).

That's why it is important to consider the change in the context of the declared API. If the behavior was not documented, then you are still promising the same things to your users, and the API is not broken.

And that is also why it is important to not rely on undeclared/undocumented behavior. If it is not explicitly declared/documented, then it can change at any patch version.

@jwdonahue
Copy link
Contributor

It really comes down to how much blow-back are you willing and able to absorb. If you make an implementation change that breaks a tiny fraction of your users, do you care? If you break enough of them, maybe you lose your job or go out of business.

The point of SemVer is to communicate risks to your consumers such that they can make informed decisions whether to test your changes. If you break too many of them with patch level updates too often, then they aren't going to trust your patch releases. The downside to that is, any legitimate security fixes you push, may not be taken up by anyone, and that could ruin your reputation beyond your direct customer base.

@Vutheavuthea

This comment was marked as spam.

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

No branches or pull requests

8 participants
@ljharb @jcornaz @jwdonahue @aman-agrawal-coolblue @Vutheavuthea and others