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

Bump node engine to 14 #1257

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Bump node engine to 14 #1257

merged 2 commits into from
Mar 22, 2022

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Mar 17, 2022

This reflects what is running on CI. and also some of the current dependences already require that version.

Using 14.13.1 for lint-staged ^12:

See:

See #1252 (comment)

@fbartho
Copy link
Member

fbartho commented Mar 18, 2022

Looks like appveyor’s matrix might need an update too

@fbartho
Copy link
Member

fbartho commented Mar 18, 2022

nodejs_version: "12"

@glensc
Copy link
Contributor Author

glensc commented Mar 18, 2022

Looks like we ran into chicken-egg problem. I wanted to bump node to 14 to be able to finish commander.js update, but updating @types/node breaks commander integration:

$ tsc -p tsconfig.production.json
node_modules/commander/typings/index.d.ts:26:25 - error TS2689: Cannot extend an interface 'NodeJS.EventEmitter'. Did you mean 'implements'?

26   class Command extends NodeJS.EventEmitter {
                           ~~~~~~

source/platforms/github/GitHubUtils.ts:92:22 - error TS2769: No overload matches this call.

I propose we move on here without @types/node bump, and do that once dependencies are updated, so the bumping can be done.

@glensc
Copy link
Contributor Author

glensc commented Mar 18, 2022

I've posted @types/node pr separately:

@glensc
Copy link
Contributor Author

glensc commented Mar 22, 2022

Who will do the merge now? I see one approval, but it was set before further commits.

image

and @fbartho you didn't remove your approval after further comments.

@glensc
Copy link
Contributor Author

glensc commented Mar 22, 2022

What is the process here? seems it's not moving ahead naturally (stalled for 4 days) and it's not described in contributing.md either other than push access is given out easily

@orta
Copy link
Member

orta commented Mar 22, 2022

PR looks good to me, and there's no CLA on people's spare time on OSS maintenance

@orta orta merged commit 6fc510f into danger:main Mar 22, 2022
@glensc glensc deleted the node-14-engine branch March 22, 2022 18:59
@glensc
Copy link
Contributor Author

glensc commented Mar 22, 2022

nevertheless no answer on merge policies. since I have the mana to merge, should I have merged myself (or who?)? would like to hear some guides or something.

@orta
Copy link
Member

orta commented Mar 22, 2022

Then no, please do not merge yourself, I + @fbartho generally merge most things - the ability to self merge is a system designed to allow for folks to worry less about their dependencies and handle absent maintainers (or folks looking to own sections of the codebase after some time)

@falkenhawk
Copy link
Contributor

this was not the best decision to bump node dependency to v14. Distributed dangerjs might still run without any issues on v12 enviroments, depends what target is used when building it. lint-staged is in devDependencies and hence v14 was required only on dev.

I bumped danger on CI which tested a package on different node versions (still would like to keep that package compatible with nodejs <14) but I'd need to either get rid of danger on nodejs v12 (even if it actually reports back on newer v14+ nodejs versions, it is still in package.json trying to be installed and CI fails)
error danger@11.2.0: The engine "node" is incompatible with this module. Expected version ">=14.13.1". Got "12.22.12"

@glensc
Copy link
Contributor Author

glensc commented Nov 30, 2022

You probably don't need to run danger for all node versions. you are most likely checking the static code committed to repo, so it doesn't matter what node version was used to run danger.js.

But staying on low versions is not a good option either. the dependencies can't be updated and the dependencies will be reported full of security vulnerabilities.

Maintaining multiple version series probably won't going to work either due lack of manpower.

my 2ç

@fbartho
Copy link
Member

fbartho commented Nov 30, 2022

@falkenhawk you're welcome to stay behind on an older version of DangerJS, the older versions are the ones that are still compatible with Node12.

NodeJS itself no longer supports Node12, and new features for DangerJS can't maintain a wider support range than that, it's just not practical. https://github.com/nodejs/release#release-schedule

What's your real concern here @falkenhawk? You're trying to upgrade to latest DangerJS, but not upgrade to latest NodeJS?

It's common when somebody is testing a special edge case, to use a pattern like patch-package to disable the node engine check on this package. We obviously can't provide support if you're patching things like that, but if you think your use-case would still be fine, you're welcome to remove that Engine check in your code-bases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants