ckb-cell / rgbpp-sdk

Utilities for Bitcoin and RGB++ asset integration
ISC License
52 stars 16 forks source link

[Discussion] Breaking changes in versioning #167

Closed ShookLyngs closed 3 months ago

ShookLyngs commented 3 months ago

Topic

Semantic Versioning suggests that when there are breaking changes (incompatible changes) in the code, we should bump the major version of the library. That means if our library is currently versioned as v0.1.0, and we change an API (its parameters, etc.) right now, our next release should contain a major bump, and it should be versioned as v1.0.0.

So here are the questions:

  1. Do you think we should follow the rules strictly?
  2. Is there any exception for v0.1.0 -> v0.2.0 even with a breaking change?

Reference

ShookLyngs commented 3 months ago
  1. Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
  2. Major version X (X.y.z | X > 0) MUST be incremented if any backward incompatible changes are introduced to the public API. It MAY also include minor and patch level changes. Patch and minor versions MUST be reset to 0 when major version is incremented.

It seems for 0.y.z versions, the rules are not that strict. It leads to another question: when should we bump to v1.0.0? Also, even even though we CAN make breaking changes while still bumping to v0.2.0, but should we?

Flouse commented 3 months ago

It leads to another question: when should we bump to v1.0.0?

A rough idea:

Assuming no breaking change is introduced for three months, v1.0.0 can be considered.

Flouse commented 3 months ago

Also, even through we CAN make breaking changes while still bumping to v0.2.0, but should we?

We should try our best to avoid breaking changes. Some strategies include:

duanyytop commented 3 months ago

Also, even through we CAN make breaking changes while still bumping to v0.2.0, but should we?

We should try our best to avoid breaking changes. Some strategies include:

  • Adding new optional fields or parameters instead of modifying existing ones
  • Introducing new functions/interfaces for new functionality while keeping old functions/interfaces unchanged

    • Deprecate old functions/interfaces gradually, and carefully
  • Using default values for new parameters to avoid breaking existing clients
  • etc.

LGTM

ShookLyngs commented 3 months ago

Let's take an example, we have a function that is requested to be updated:

-declare function aToB(a: A): B | undefined;
+declare function aToB(a: A): B;

The updated signature has no undefined in the return type. Do you think we should perform the breaking change? If yes, how?

Some additional factors:

  1. Our package version is under 0.y.z
  2. The API is likely barely used on the user-side, relatively safe to change
ahonn commented 3 months ago

Let's take an example, we have a function that is requested to be updated:

-declare function aToB(a: A): B | undefined;
+declare function aToB(a: A): B;

The updated signature has no undefined in the return type. Do you think we should perform the breaking change? If yes, how?

Some additional factors:

  1. Our package version is under 0.y.z
  2. The API is likely barely used on the user-side, relatively safe to change

This example seems to be compatible, B is a subset of B | undefined

ShookLyngs commented 3 months ago

This example seems to be compatible, B is a subset of B | undefined

The internal logic will be updated:

function aToB(a: A) {
-  if (cannotConvert(a)) { return undefined; }
+  if (cannotConvert(a)) { throw new Error('xx'); }
  ...
}

And here's the usage update:

-const b = aToB(a);
-if (!b) { ... // route 1 }
-... // route 2

+try { 
+  const b = aToB(a);
+  ... // route 2
+} catch (e) { ... // route 1 }
ahonn commented 3 months ago

I think throwing an error is acceptable because that can throw any error before. The return type B is a subset of B | undefined, So I think it is compatible. If we bump to 0.2.0, just a remark in the changes log will be fine.

ahonn commented 3 months ago

The other option is to provide a new function that will throw an error, keep the old one, and mark it as Deprecated. Remove and use the new function instead of the old one in the next version.

function aToB(a: A): B | undefined {
  // ...
}
function convertAToB(a: A): B { // Or maybe `strictAToB`
  const b = aToB(a);
  if (b === undefined) {
    throw new Error();
  }
  // ...
}

But I still think just change & remark will be fine.

ShookLyngs commented 3 months ago

After discussions, the conclusion is to avoid breaking changes as much as we can, unless the change has a high priority.