azure-sdk-for-js icon indicating copy to clipboard operation
azure-sdk-for-js copied to clipboard

breaking change in v1.6.0: dropping node versions is semver-major

Open ljharb opened this issue 1 year ago • 15 comments

Even just the engines declaration here is a breaking change, and should have been v2. Please revert, and publish a new v1, and publish a v2 if you want to drop node < 18.

ljharb avatar Feb 02 '24 17:02 ljharb

@ljharb thank you for the report! Quoting our support policy here:

The Azure SDK libraries for JavaScript will not be guaranteed to work on Node.js versions that have reached their end of life. Dropping support for such Node.js versions may be done without increasing the major version of the Azure SDK libraries.

jeremymeng avatar Feb 02 '24 17:02 jeremymeng

I see that you've documented it, but that means you're not following semantic versioning, which requires increasing the major here.

ljharb avatar Feb 02 '24 18:02 ljharb

@ljharb I hear you that one interpretation of semver is to consider updating runtime support to be a breaking change. However, since we support multiple runtimes that have well-known support cycles (such as Node's LTS schedule), we have chosen to focus on breaking changes to the public API surface we provide.

This is in part to ensure that for those who are keeping their environments patched are able to get the latest fixes to the code that we own rather than forcing a developer to look at our CHANGELOG.md only to find out that no change within the package itself merits their consideration.

From https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api:

What should I do if I update my own dependencies without changing the public API? That would be considered compatible since it does not affect the public API.

As we do not consider runtime globals or syntax provided by the browser/node/etc to be part of our Public API, it is not considered a breaking change if we depend on features that are supported by our supported runtime matrix.

I hope this helps clarify our position and I strongly recommend you use a version of Node that is maintained due to the lack of security fixes for unmaintained versions.

xirzec avatar Feb 02 '24 20:02 xirzec

Jordan!! Nice to see you on my side of the world 😁

Can you say more about the impact the engines change/dropping node < 18 had on you? E.g. do you have node 16 deployments still?

bterlson avatar Feb 02 '24 20:02 bterlson

@xirzec your interpretation contradicts the one the entire node ecosystem users, despite following the letter of the semver spec, and imo that's far more important.

@bterlson howdy! :-) basically, these azure packages are used in github's @actions/ packages, which i'm using in a github action, some parts of which still run in node 16. I plan to upgrade it to node 20 soon, but this means i can't update a patch version of the @actions package because it assumed, like everything in node does and should, that engines would never be dropped in a non-major.

ljharb avatar Feb 02 '24 21:02 ljharb

I'm sorry, that does sound frustrating. That said, I don't think we will be changing our approach in general. As you say, the letter of the law as it were is not super relevant, what matters is what our users need from our packages, and weighing tradeoffs with that in mind. And our experience has been that incrementing major creates a large burden on our consumers and various undesirable outcomes that outweigh the downsides of dropping unsupported node versions in a minor release. AWS and GCP have come to similar conclusions for their JS libraries. I have also observed other projects following this general process in my travels, so while I can't comment on the overall prevalence of this approach, I feel confident in saying it is not unheard of.

One thing we could consider is dropping unsupported nodes less aggressively. That is something we can look in to!

bterlson avatar Feb 02 '24 22:02 bterlson

Certainly if you'd only gone to >= 16, which is what github.com supports in actions, then I wouldn't have even noticed :-)

ljharb avatar Feb 02 '24 23:02 ljharb

That's fair, thank you for letting us know about your experience too, super valuable!

bterlson avatar Feb 03 '24 02:02 bterlson

This is in part to ensure that for those who are keeping their environments patched are able to get the latest fixes to the code that we own rather than forcing a developer to look at our CHANGELOG.md only to find out that no change within the package itself merits their consideration.

I don't see this as a big problem, since the node's LTS schedule is 30 months. What I mean is that developers would only have to check the changelog effectively every 2.5 years just when they were also about to update the version of Node itself, since its end of life would have reached. Well, that's my point of view, we also had our pipelines affected due to the use of appcenter-cli running on the reactnativecommunity/react-native-android:8 image that uses Node 16, we upgraded to version 9, which already uses Node 18, for us it wasn't a big problem, but for some it could be

Gabriel8579 avatar Feb 05 '24 14:02 Gabriel8579

Another reason we moved to NodeJS 18 quicker is that EOL of NodeJS 16 was aligning with EOL of OpenSSL 1.1.1. It's not wise to depend on an unsupported security component.

jeremymeng avatar Feb 05 '24 17:02 jeremymeng

More information on why node16 reached EOL earlier than usual: https://nodejs.org/en/blog/announcements/nodejs16-eol

xirzec avatar Feb 05 '24 21:02 xirzec

ftr, semver doesn’t pivot on wisdom or support or security, only on breakage :-)

ljharb avatar Feb 05 '24 21:02 ljharb

I want to check my understanding about what is being proposed.

It sounds like whenever we adjust our engines entry we would also major version our packages.

However, since we don't support the old runtime anymore, I think that would mean we would have to npm deprecate the previous released major.

This means now instead of resolving an engines warning from npm, you have to resolve a deprecated dependency warning from npm.

Does this make your life any better? Aren't you basically in the same place?

xirzec avatar Feb 05 '24 22:02 xirzec

@xirzec no, there’s never a requirement to deprecate something merely based on not supporting it.

Additionally, deprecation earnings are just warnings - they don’t obstruct, and everyone just ignores them - so it costs nothing. Engines limits fail installs.

ljharb avatar Feb 05 '24 23:02 ljharb

Several of our services are still using node 16 and had these packages used upstream (not direct deps). Moving without a major version broke our builds as ^4.6.0 for @azure/keyvault-secrets now installs 4.8.0 which is node 18+ only.

Obviously, yes we should have upgraded from node 16 at this point. However, a few legacy enterprise services exist still and given the complexities of build pipelines and the like it isn't always doable to upgrade mission critical services arbitrarily at scale without a huge amount of additional testing in production environments.

Given Azure's primary target market is enterprise customers (often big and slow at updating) this seems like an odd move to break semver. It should have gone to 5.0.0 as a major bump.

The good news is that we can fix this using package.json resolutions but it's a huge hassle to go into every repo we operate to add resolutions to fix the issue after the builds fail.

Regarding the Support Policy

The Azure SDK libraries for JavaScript will not be guaranteed to work on Node.js versions that have reached their end of life. Dropping support for such Node.js versions may be done without increasing the major version of the Azure SDK libraries.

Personally, I think this needs rewording. Not guaranteed to work is one thing and I believe many companies could accept that. However, changing the engine requirement is forcing it not to work. Big difference IMHO.

joshmeads avatar Apr 26 '24 22:04 joshmeads

Hey folks - apologies again for the inconvenience caused by the recent changes in Node.js version support and engines field, and thanks for bringing this up and continuing to share your feedback!

We understand that this has been disruptive and should have been communicated more effectively.

In response to this, we are making the following changes to our process and policies:

  • We will be more conservative updating the engines field. While we will maintain our current versioning policy, we will not drop engines support until the ecosystem has broadly moved to a non-EOL runtime by default. This may include examining GitHub Actions, Azure Web Apps, Azure Functions, and others.
  • We will announce engines retirements 3 to 6 months beforehand. We will announce these changes via:

While it can be argued that dropping engines support for unsafe or EOL runtimes is a breaking change worthy of a semver major release, unfortunately major releases come with significant tradeoffs for some of our users. Given this, we will continue with our current versioning strategy with the above modifications and see if further changes are necessary.

Please continue to share your thoughts on this issue, this discussion has been super valuable. We really appreciate it!

maorleger avatar Jun 03 '24 20:06 maorleger

To be clear, it's not "can be argued", it's the letter of the spec, and https://github.com/Azure/azure-sdk-for-js/pull/29895/files#diff-c5fe610b0215b144474106251cab954f8af55fe26cbd5d2265fd6ca19109c97bR36 means that this library is not following semver.

You're certainly free to do that! But, it would be helpful to consumers to make it clear which versioning strategy is in use, and it seems that it's not semver.

ljharb avatar Jun 17 '24 17:06 ljharb