Remove engines from package #399
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #399 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 379 460 +81
Branches 136 215 +79
=========================================
+ Hits 379 460 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Why did it work before? What version of Vitest were they using before? Or did it always use the latest, and at some point something broke because of Vitest?
Really, whether the engines field is kept or removed, it should still respect the CI support, which was Node 16. It’s kind of unlikely for path-to-regexp to break, but not impossible
|
Why did what work before? Vitest was upgraded to fix a security vulnerability, the latest vitest is node 18+. Actual node version support is pretty meaningless in this package, as there's nothing related to node used and through TypeScript checks the ES version is enforced. |
There was a problem hiding this comment.
LGTM! Just to clarify for those following the engines ADR discussion. The consensus seems to be not to treat removing engines as a major change (see: expressjs/discussions#289 (comment)).
This PR is aligned with that proposal 👍
Engines was bumped in #348 to support the latest vitest version, removing the engines field instead to release a new minor with the changes on main. I could alternatively revert to
>=16but I don't want it out of sync with CI or to figure out how to downgrade vitest for this.