nodejs / import-in-the-middle

Like `require-in-the-middle`, but for ESM import
https://www.npmjs.com/package/import-in-the-middle
Apache License 2.0
72 stars 27 forks source link

feat: Add package.json engines to pin Node 12+ #119

Closed AbhiPrasad closed 2 months ago

AbhiPrasad commented 5 months ago

We should make it clear that this package only supports Node 12+. This also allows us to more easily bump the minimum Node version for future majors.

AbhiPrasad commented 5 months ago

I picked 12.x because this what was in the matrix: https://github.com/nodejs/import-in-the-middle/blob/537cbad6de0f8c186473b2f7d7092c132625e0b2/.github/workflows/ci.yml#L28

Is there another place I should be looking?

For that reason, I wouldn't anticipate wanting to drop support for older versions of Node.js

I think this is very reasonable, and aligns with my personal (and professional) philosophy around this stuff.

Practically speaking though ESM is quite the broken experience in general if you use any version below 18, (import.meta.resolve is only 20.6+ for example 😬), so I'm a little more comfortable being aggressive with version support because I don't assume there are many Node 12, 14, 16 apps with ESM. I would even be fine to sync up completely with OpenTelemetry (they are 14+), and adopt the same node version cadence as them because I feel like they are the primary persona this package needs to support.

What's nice is that we have all the vendors here as well, so we can easily query our systems to figure out what adoption levels are like for various Node versions to understand the ramifications of dropping node support for things.

timfish commented 5 months ago

I picked 12.x because this what was in the matrix

12.x means that the latest CI runs have actually been using the latest v12 release (ie. v12.22.12) rather than 12.0.0, so maybe that's a good minimum?

AbhiPrasad commented 4 months ago

I went ahead and added 12.0.0 to the matrix. I figured this would be better because it doesn't add much to CI time, and have a more liberal engines config is easier to understand for users.

Qard commented 4 months ago

@jsumners-nr That's a nice idea, but that's not reality for many of our customers. There are many companies locked to a Node.js runtime version but able to add and upgrade individual modules within their apps. They expect to have access to our new product features and so upgrade the libraries but still expect it to work with their old runtime versions.

As much as I would like to be able to drop support for EOL versions, it's not really something we can do.

trentm commented 4 months ago

My notes on which minor/patch versions of which Node.js majors we support using IITM with the Elastic APM agent, in case it helps:

// Supported Node.js version range for import-in-the-middle usage.
// - v12.20.0 add "named exports for CJS via static analysis"
//   https://nodejs.org/en/blog/release/v12.20.0
// - v14.13.1 includes "named exports for CJS via static analysis" plus some
//   fixes in the .1 minor release
// - v18.1.0 fixes an issue in v18.0.0
//   https://github.com/nodejs/node/pull/42881
// - `^18.19.0 || >=20` support was added by IITM@1.7.3
//   https://github.com/DataDog/import-in-the-middle/pull/27
// - v20.2.0 fixes an issue in v20
//   I think it is https://github.com/nodejs/node/issues/47929
trentm commented 4 months ago
  • The engines block does not do anything except incur maintenance cost.

Over my time doing do maint on APM libs that have a zillion testing deps (e.g. for every module that is instrumented), I have found that the "engines" block is a very helpful signal from the package authors for which versions are supported. When updating to new versions the "engines" value is included in the "package-lock.json" diff (always? typically? npm package-lock details are under-documented), which I find helpful during review.

Some projects have some CI file that hints at the supported Node.js versions. Sometimes that list of versions is indirectly in a shared GH action, or in the meta file for some other CI system.

Granted "engines" is advisory, so it is never a perfect signal. I've not found it a significant maintenance burden to update.

  • Tracking older than currently supported core LTS versions is too expensive. If folks insist on running on unsupported versions, they should be comfortable running unsupported packages that were released alongside those Node.js versions.

I disagree, in general. Sometimes supporting older versions is difficult for some libs, sometimes not. If IITM were to drop support for Node.js 14 and 16 right now, then OpenTelemetry and Elastic (for example) would feel the need to fork it the first time a needed bug fix came alone.