search

danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.22k stars 363 forks source link

Upgrade node engine to >=18 & @types/node to ^18.19.14 #1424

Closed heltoft closed 5 months ago

heltoft commented 5 months ago

The currently used node environment has reached end of life. Updating to 18.x which is the version with Maintenance status for Node.js.

This change is a prerequisite to start solving some of the current GitLab issues, especially in regards to problems with inline comments which needs dependency updates.

orta commented 5 months ago

I'm not onboard with forcing all users of danger to need node 18+ in order to get updates. I have no problem with making that the case for contributing to this repo though!

heltoft commented 5 months ago

Hey @orta, do I understand you correctly that you are ok with updating if there is some contributions to go along with the update? If so, then I can assure you there is more changes coming 🚀

I have been working on getting inline comments to work on GitLab recently in my spare time. Mainly focused around the following issues:

1386

1412

1405

I have a version of danger-js running where these issues are solved that we have been using for a few weeks where I work, and it has been working well for us.

However it is quite a lot of changes so what I am trying to do is to split up the changes into smaller chunks 🙏

How would you suggest for me to proceed in this case?

orta commented 5 months ago

Sure, 1 PR is reasonable for that scope

Is the underlaying issue for the engines change that the gitbreaker update is going to force us to make our consumers update also?

heltoft commented 5 months ago

Alright sounds good. I have a few more tests to write, but I will create an MR with all the changes when I feel it is ready 🙂

Yes gitbeaker requires node 18 since it leverages native fetch under the hood in newer versions. The documentation for gitbeaker states that it is possible to make it work with a polyfill for node 16.18+, however I haven't been able to make that work so far, and it would also require to use the --ignore-engines flag. And if we are already bumping node engine I personally think it makes more sense to go to 18 as that is the last maintained version.