search

lost-pixel / lost-pixel

Open source alternative to Percy, Chromatic, Applitools.
https://lost-pixel.com/
MIT License
1.35k stars 46 forks source link

Breaking change in requirements in minor version #427

Open mabinogi opened 3 weeks ago

mabinogi commented 3 weeks ago

Bug description

Version 3.20.0 updates the engines value in package.json from requiring (the still in LTS) node 18, to requiring the (not yet in LTS) node 22.

from the Node releases page:

Major Node.js versions enter Current release status for six months, which gives library authors time to add support for them. After six months, odd-numbered releases (9, 11, etc.) become unsupported, and even-numbered releases (10, 12, etc.) move to Active LTS status and are ready for general use. LTS release status is "long-term support", which typically guarantees that critical bugs will be fixed for a total of 30 months. Production applications should only use Active LTS or Maintenance LTS releases.

Increasing the node requirement in engines is a breaking change to start with - ie, existing users of this package on Node 18 or Node 20, not only can't use it, but it totally breaks their ability to even install other dependencies simply because this package has marked node 22 as a requirement. Obviously if lost-pixel genuinely makes use of features only available in Node 22, then updating the engines field is required, but it should be in a Major release, so that consumers of the package don't suddenly face broken builds.

Node 22 is not yet in Active LTS, so as in the text above, it is not encouraged to use it in production yet, so this change will most likely affect every serious user of lost-pixel that actually pays attention to the recommendations of the providers of the platform.

I'd suggest reverting the engines change until you're ready to do a new major version - and if the change was because of the use of features only in Node 22, you'll need to revert them too, release a fixed 3.x version, and then start on 4.x with the new changes, or detect the use of Node 22 to enable the feature that depends on it, preserving compatibility for more of your users without having to leave them behind.

And if there aren't other changes that require the use of Node 22, then there's no need to change engines at all.

How to reproduce

  1. Remove yarn.lock or package-lock.json and rerun yarn or npm install. (No need to even explicitly update lost-pixel)

Result:

error lost-pixel@3.20.0: The engine "node" is incompatible with this module. Expected version ">=22". Got "20.9.0"

Expected behavior

yarn or npm should complete without errors if they previously did on an earlier release with the same major version.

Lost Pixel information

lost-pixel logs from CI

No response

n2o1988 commented 3 weeks ago

Node 22 is not yet in Active LTS, so as in the text above, it is not encouraged to use it in production yet, so this change will most likely affect every serious user of lost-pixel that actually pays attention to the recommendations of the providers of the platform.

+1 on this. Requiring Node22 at this early stage sounds too strict and this update would force us to stop using this library. As said above, you should only bump the minimum engine in package.json if the library genuinely use features that are only available there.

chriskalmar commented 3 weeks ago

I'm sorry, that wasn't a good decision. I was too hasty with upgrading Node. 😬 You are right, there's no absolute need to switch to 22 just yet.

Fixed it in https://github.com/lost-pixel/lost-pixel/pull/428 by going back to Node 18.

Also, released a new version: https://github.com/lost-pixel/lost-pixel/releases/tag/v3.21.0 https://www.npmjs.com/package/lost-pixel/v/3.21.0

Please let me know if things are working for you again.