Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(package.json) Explicit drop support for node<18 #59

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mureinik
Copy link

@mureinik mureinik commented Sep 5, 2024

Followup to #54:
That PR added a statement that support for node<18 is dropped, and amended the CI accordingly.

This PR builds on that and makes this support statement explicit in the package.json.

Followup to pillarjs#54:
That PR added a statement that support for node<18 is dropped, and
amended the CI accordingly.

This PR builds on that and makes this support statement explicit in
the package.json.
@wesleytodd
Copy link
Member

I knew someone would see this and open this PR. I did this intentionally. We are in a process of fixing nearly 10 years of stagnation and are attempting to make the most smooth transition we can for consumers. Since we did not introduce any changes that actually BREAK those versions I have chosen to not change the engines field. This means that we are a bit more free to steward folks forward without putting in arbitrarily roadblocks in the upgrade path.

That said, we do not intend to ship fixes for anything other that security issues to support node versions older than 18. We are working on docs and stuff for this, but it takes time and we are focused on getting the code released and stable first. So, happy to hear your opinion on this, because I anticipated it being controversial.

@mureinik
Copy link
Author

mureinik commented Sep 5, 2024

Egg on my face!

I was not aware this was intentional - #54 didn't mention it, and I thought it was just an oversight.
Apologies for the noise.

@wesleytodd
Copy link
Member

No apologies necessary! I was quite concerned this decision would be seen as the wrong way and I didn't want to cause more confusion by trying to preemptively explain my thinking in case it was never going to become an issue for folks. In open source it is sometimes hard to tell what folks will care about and what they will not even notice, and with the workload we have taken on trying to get these packages unstuck, I was trying to optimize for maintainer time over correctness.

@wesleytodd
Copy link
Member

We merged this change in at least one other package. While this is considered a breaking change in the strictly technical sense, I think we may merge it since it is in line with the support policy we are taking and will be publishing other packages with this change. I will leave this comment here for a bit to see if anyone wants to block this from moving forward. If not, we can merge and release.

@ljharb
Copy link

ljharb commented Jan 16, 2025

As long as it's semver-major, it's fine :-) this isn't just in the technical sense, it will actually break things in a real sense, since many setups do check the engines field.

@wesleytodd
Copy link
Member

This was my original argument, but I didn't want to stand in the way of progress and a few other packages (I can get links) did merge this change with the intent to release as a minor. I can't remember off hand if any of those have been released yet, but we have not had anyone willing to take a strong stance against releasing them.

To be clear, I believe we would be better off removing the engines fields from all packages except express and just documenting well the minimum supported version. That is not because I think it is the most technically correct, it is because I think it causes the least harm to users and maintainers. That said, in the discussion I was really the only person in favor of that path IIRC so I didn't fight it.

@ljharb
Copy link

ljharb commented Jan 16, 2025

I definitely disagree; keeping both the engines field and semver accurate is what does the least harm. What's the issue with a major bump of this package?

@wesleytodd
Copy link
Member

Not a whole lot of resistance to that idea, it was just not mentioned here before. I believe it was @ctcpip who was the strongest proponent of just releasing these as minors (correct me if I am wrong on that Chris).

@ljharb
Copy link

ljharb commented Jan 16, 2025

Whenever someone does that, it breaks me, fwiw - I'd strongly urge you to either fully drop compat (including engines, in a semver-major) or not drop compat. Nobody wants semver z̲̗̼͙̥͚͛͑̏a̦̟̳͋̄̅ͬ̌͒͟ļ̟̉͌ͪ͌̃̚g͔͇̯̜ͬ̒́o̢̹ͧͥͪͬ

@ctcpip
Copy link
Member

ctcpip commented Jan 16, 2025

please see: expressjs/discussions#286 (comment)

@ctcpip ctcpip marked this pull request as draft January 16, 2025 22:17
@ctcpip ctcpip added the v3 label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants