hubotio / hubot

A customizable life embetterment robot.
https://hubotio.github.io/hubot/
MIT License
16.66k stars 3.75k forks source link

Align to Node.js supported versions #1619

Closed joeyguerra closed 1 year ago

joeyguerra commented 1 year ago

Proposal

Let's align to Node's End of life schedule for version support.

@levenleven. I created an issue to discuss and plan a release.

xurizaemon commented 1 year ago

Makes sense to me! It helps to know what we're agreeing on as "supported", since support may be interpreted differently. For me this means something like:

To me it doesn't mean that we introduce code to prevent Hubot running on unsupported Node.js versions (warning messages are fine, and the engines entry will trigger a warning at runtime if Node.js is not aligned to the Hubot release).

I suspect adding an upper bound on the engines entry will only create work; new versions will come out and folks will say "Hubot doesn't install", because the chore of bumping the max engine value hasn't yet been done? So I propose "engines": {"node": "> 14"}.

Likewise, in 7d91ceefbf0eac495e425cc70091bf69ccd946b7 we introduced a test matrix of [ 14.x, 16.x, 18.x, latest ], currently against Linux, Windows and macOS runners. I used latest there so that when a new release appears in Github's list of Node versions, our coverage lifts automatically.

When Node.js v14 hits EOL (less than a week away), there's a chore to bump the version, but it's not urgent. Our motive for defining a supported range is to provide clarity of what we think works, not to police how it's consumed. Lots of folks run old versions of things ... what we do is disable test coverage for that version and bump the supported versions. Existing bots should continue to run, and will start to warn about Node.js if/when the hubot dependency is updated in that bot install.

joeyguerra commented 1 year ago

I agree with all of the above except I'd like some clarity on:

To me it doesn't mean that we introduce code to prevent Hubot running on unsupported Node.js versions (warning messages are fine, and the engines entry will trigger a warning at runtime if Node.js is not aligned to the Hubot release).

Is another way to say this is that we don't add code that checks what Node.js version is running AND throw an exception if it's not within the supported range?

xurizaemon commented 1 year ago

I think I would generally avoid checking against a version at all unless there's no way to detect feature support 👃🏼

And I think a "bail out if running on unsupported version" code path is redundant. Leave it to throw an exception when something illicit happens.

(Maybe if there were a ton of people filing bug reports against outlandish old node versions and there was a need to send a signal ... maybe.)

levenleven commented 1 year ago

I think mentioning the minimal supported node version in readme (or just linking the End of life) and corresponding test runner matrix coverage is good enough. We could probably skip the engines setting, if folks are using it with older node version - shouldn't bother us

joeyguerra commented 1 year ago

Ok. This all sounds good to me too. I was hoping to get the other chore PRs merged and released before implementing this plan.