Plans to support Semantic Versioning

[phillycheeze]

Currently, the releases for the SDK always iterate a patch version for bug fixes and changes, and a minor version for added features.

Describe the solution you'd like
A proper semantic versioning process where patch versions are backwards compatible and only fix bugs. They do not introduce breaking changes.

Describe alternatives you've considered
Continue down the current path of versioning.

Additional context
The latest 5.9.2 release fundamentally changed how the update handler worked, causing it to likely break for people who upgraded. This seems odd since going from 5.9.1 -> 5.9.2 should not be changing core functionality. There are other examples of this in the releases over time.

[eli-darkly]

Hi - thanks for writing. Before getting to the specific issue of the update handler, I'd like to address the more general issue of how we approach versioning.

The semver standard is conceptually straightforward, yet also open to interpretation in some areas because it uses general terms like "new functionality", "incorrect behavior", and "public API". Not everyone will agree on what is new functionality, what is incorrect, and what is the API. One can find many vigorous online discussions of those principles, without a strong consensus. So, our own guidelines have been:

• If a behavior was unintended— or, was intended at the time, but was later found to be problematic— and was not documented, and fixing it does not change any documented types, properties, methods, or parameters... then we consider it to be appropriate for a patch release.
• "New functionality" does not mean any new behavior, it means providing the ability to do something with the SDK that you could not do before, involving a new type, property, method, optional parameter, or supported parameter value.
• A "breaking change" means that a type, property, method, parameter, or allowable parameter value is no longer defined or will no longer produce the desired behavior as documented, so that the application code will not compile (in a compiled language), or will produce a runtime error, or will be unable to accomplish the overall desired behavior. ("Documented" in this case means both any literal documentation we have written, and also the simple existence of symbols in the public API. On a platform like Node.js that does not have the same kind of public/private visibility system as in for instance Java or .NET, internal methods or properties may exist that we cannot prevent application code from accessing; but if they are excluded from all documentation and from explicit interface declarations such as TypeScript declarations, then they're considered non-public.)

One thing we've had to take into account is that the SDK is not a self-sufficient piece of software; much of its functionality is provided by the LaunchDarkly web services. The internal API and data model that the SDK uses to communicate with the services is not part of the SDK's own API. So if for instance we added a bandwidth optimization to drop certain properties from the flag data model if their values can be inferred by default, and this does not have any effect on how application code would interact with the SDK, that is not a breaking change in the SDK. This is particularly relevant to the update event issue as I'll get to in a moment.

I'd like to emphasize that if (as you said) you have other examples of problematic use of versioning in the past, please let us know about them. What I've described above are the principles we've intended to follow. If we have made mistakes with it in the past and not acknowledged them in a follow-up fix, we need to hear about it. If you disagree with the principles, we need to hear about that too. It is hard to guess from your general reference to "changing core functionality" what specific changes you might mean.

[eli-darkly]

Now, regarding the change in update event behavior. Our rationale for considering this a correction to incorrect and undocumented behavior, and therefore allowable for a patch release, was as follows:

• The only thing the documentation had previously said about the parameter passed to the event handler was this: "The event parameter is an object containing the flag configuration; its key property is the flag key." This was a late addition to the docs; until recently the update event feature was not documented in any way at all.
• "The flag configuration" is not part of the SDK API and is not documented anywhere therein. That is deliberate, because as I mentioned in my first comment, that data model is subject to change due to changes in the LaunchDarkly service which may or may not require SDK code changes and do not change the public semantics of evaluating flags. In all of our SDK designs, a feature flag is represented as a black box that has a key and produces a value; we specifically do not provide any APIs for an application to pass a full flag configuration to some method and make it do something. It is always possible to see that configuration if you query the REST endpoints of the LD service, but the whole point of the SDKs is to hide those details.
• The fact that it was sending the full configuration object was in fact a mistake on our part. Not only is it exposing undocumented and unsupported data model properties, but it is unsafe because that is a mutable object that lives in the SDK's internal data store; if for some reason we did want to pass it to the application, a deep copy would be required.
• It is still the case that the event parameter is an object whose key property is the flag key.
• If the disappearance of other data model properties from that object were to be considered a breaking change, then we would have to increment the major version every time the flag model used by the LD service changed. That is highly undesirable since, for developers who are using the SDK in the intended and documented way where flags are evaluated simply by specifying the key and the user, we want them to receive updates to this functionality transparently and not to fear that they will have to change their application code for this new version. So we really do not have the option to keep exposing that object, unless we decide to create an actual supported, documented, public API whose versioning is separate from the SDK versioning.

[phillycheeze]

Thanks for the thoughtful and quick reply @eli-darkly ! Definitely not trying to say that this was intentional or the process was flawed.

In your snippet of a breaking change, it goes on to list a series of items that I interpret as meaning that if any of them are true:

A "breaking change" means that ......... , or will produce a runtime error,

Obviously this just goes into semantics which doesn't really matter too much. It does seem the documentation on your end said otherwise, and the functionality wasn't meant to be that way. So I definitely understand why it was fixed and I agree that it was the best move. I just wish it would've been a minor version is all.

We set the dependency as ~5.9.0 in our package.json as we do with most dependencies. I've always considered it a general best practice as something like a runtime error should never pop up when upgrading a patch version. Luckily, we found this out it in our staging environment during a test and have since just hardcoded the version to 5.9.1 until we can re-write how we use that handler. Makes me a little worried going forward as next time our org might not be so lucky and catch it in staging, which we usually don't tbh :)

Maybe we just hit a tough edge-case here! So I'm glad to have this discussion. Please let me know your thoughts overall. LD is honestly better about semantic versioning than a lot of companies/packages out there, so hopefully my initial comment wasn't coming off as criticizing it.

[eli-darkly]

Well, I think the only reason any disagreement is possible on this is that "will produce a runtime error" is a very, very broad statement. I should probably have written that sentence a bit more specifically, but it was already getting pretty verbose.

What I mean is... if you are using supported and documented API semantics, then indeed, neither a patch version upgrade nor a minor version upgrade should produce any runtime errors. That's absolutely reasonable.

However, if you're relying on some detail that is unsupported/undocumented, and asserting that it will not change or go away... it's very hard to guarantee that there couldn't be a runtime error of that kind. For example, suppose there was an API that returned some kind of unique string identifier, and it was described only as a string, but the implementation happened to produce only alphanumeric strings. Someone might notice this, and assume it was safe to write some application code that used the string as a JSON property name. If the implementation then changed so that these strings could include characters that aren't valid in a JSON property name... that application might end up having a runtime error, but it would not have been a breaking API change in the usual sense. We felt that the specific properties in that flag object fell into that category: the content of data that had not been described as having any particular schema or constraints (other than, in this case, key).

We certainly could've been clearer about this in the documentation— I believe what happened was that someone noticed the event hadn't been documented at all, and reverse-engineered a description that confusingly mentioned "the flag configuration". We apologize for the inconvenience, and I hope that these comments at least provide some reassurance that we won't be doing these things in an arbitrary way.

[phillycheeze]

Thanks for the explanation, super helpful! In your example with the unique string identifier, I definitely agree that you obviously cannot guarantee that a runtime error won't come up. Especially with your example where someone made a wrong assumption.

In a world where documentation is often not kept up-to-date or contains errors, I think we just had assumed that this function handler was meant to return this extra data. We ended up building the solution we did with the expectation that the data was available. Definitely some fault on our end for making that assumption.

I've been convinced and I'll close this issue. :) Thanks again for the explanations...appreciate the quick and detailed responses.