elastic / elastic-transport-js

Transport classes and utilities shared among Node.js Elastic client libraries
Apache License 2.0
4 stars 24 forks source link

Upgrade of Undici is a breaking change #90

Closed jsumners-nr closed 3 months ago

jsumners-nr commented 3 months ago

https://github.com/elastic/elastic-transport-js/pull/85

That PR updates Undici from 5.22.1 to 6.7.0. This results in breaking on Node v16 because ReadableStream is not a global (see https://github.com/nodejs/undici/issues/3049).

The upgrade of Undici should be reverted and pushed to a new major.

JoshMock commented 3 months ago

As per the README, our clients and their related libraries follow the Elasticsearch versioning and release schedule, so dropping support for EOL versions of Node.js will happen between minor version releases.

If you need to continue running an EOL version of Node.js, we recommend setting your @elastic/elasticsearch version in package.json using ~ (most recent patch release for your minor) rather than ^ (most recent minor release for your major).

jsumners-nr commented 3 months ago

If you need to continue running an EOL version of Node.js, we recommend setting your @elastic/elasticsearch version in package.json using ~ (most recent patch release for your minor) rather than ^ (most recent minor release for your major).

That isn't going to fix things at this time. This is a transitive dependency of @elastic/elasticsearch and is qualified there with a ^. Without a new release of @elastic/elasticsearch there isn't a release to pin that will not include this broken dependency.

JoshMock commented 3 months ago

True enough. In that case, you can use an override in your package.json:

"overrides": {
  "@elastic/elasticsearch": {
    "@elastic/transport": "~8.4.1"
  }
}

I understand that's not ideal and will see if it's reasonable to make future versions of the client depend on the latest patch rather than latest minor of @elastic/transport.

That said, Node.js v16 reached end of active support 17 months ago, and end of security support 6 months ago, so it would be a good idea to consider that upgrade as soon as is reasonable, regardless of this particular concern.

jsumners-nr commented 3 months ago

I understand all of that and agree with you. But it is not okay to make breaking changes in a minor version regardless of those facts.

JoshMock commented 3 months ago

I totally get it. We do our best to still follow semver despite being bound to ES's release schedule, but there are multiple years between major versions of ES, which would put us in an even worse position if we didn't drop Node.js versions. If we didn't drop support for older Node.js versions between minors, the client would still have to support Node v12. We'd likely have to pin to several older, less secure versions of the client's dependencies in that situation.

An alternate option, of course, would be for this library to not follow the ES release schedule, but that's a decision that would require a large effort to undo at this point. :slightly_smiling_face:

JoshMock commented 2 months ago

Just as a follow-up: over in https://github.com/elastic/elastic-transport-js/issues/91 I decided to publish patch versions of older 8.x versions to set a patch-only dependency (e.g. ~8.4.1 instead of the current ^8.4.1), to help ease the burden for folks on older versions of Node.js.

jsumners-nr commented 2 months ago

Speaking independently of anyone else, it seems to me that you're just making more work for yourself. I suppose it looks pretty if the Node.js module has the same major version number as the ElasticSearch product, but it's really not possible due to this exact situation. You should be able to publish majors when necessary.

JoshMock commented 2 months ago

I agree. The precedent/requirement was set across all of our official language clients, long before I took over as maintainer of this project. There are benefits to users being able to easily determine compatibility with their version of ES, but it inevitably results in problems just like this.

kirrg001 commented 2 weeks ago

I understand all of that and agree with you. But it is not okay to make breaking changes in a minor version regardless of those facts.

100% agree with that.