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

Specify engine required by this package #1252

Merged
merged 1 commit into from
Mar 16, 2022
Merged

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Mar 16, 2022

Needed to set baseline when updating dependencies:

The version was chosen based on @types/node in package.json

@glensc glensc self-assigned this Mar 16, 2022
@orta
Copy link
Member

orta commented Mar 16, 2022

Looks reasonable to me, thanks

@orta orta merged commit 9337fac into danger:main Mar 16, 2022
@glensc glensc deleted the package-engine.node branch March 16, 2022 09:04
@glensc
Copy link
Contributor Author

glensc commented Mar 16, 2022

So, now that it's enforced, appears that installed packages are not compatible.

should downgrade the packages?

danger-js ☸ test(main) ➜ yarn
yarn install v1.22.17
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
error @typescript-eslint/[email protected]: The engine "node" is incompatible with this module. Expected version "^12.22.0 || ^14.17.0 || >=16.0.0". Got "10.24.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
danger-js ☸ test(main) ➜ yarn --production
yarn install v1.22.17
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
error @typescript-eslint/[email protected]: The engine "node" is incompatible with this module. Expected version "^12.22.0 || ^14.17.0 || >=16.0.0". Got "10.24.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
danger-js ☸ test(main) ➜

@glensc
Copy link
Contributor Author

glensc commented Mar 16, 2022

I've tried to downgrade, but still haven't reached a point where it is satisfied.

and probably want tooling to be several versions old?

➜ yarn add --dev '@typescript-eslint/parser@<5' 'eslint@<8' 'eslint-plugin-jest@<25' 'eslint-plugin-jsdoc@<34' '@typescript-eslint/eslint-plugin@<5.8'  'comment-parser@<1.2.0'
yarn add v1.22.17
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">= 12.0.0". Got "10.24.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

@fbartho
Copy link
Member

fbartho commented Mar 16, 2022

@glensc imho, it’s impractical to support node <14 at this point. Even our testing can’t really support it due to interlocking dependencies in various places.

In this PR: #1189 I was forced to move us to only explicitly supporting >= node 14 in our test environment.

Please don’t downgrade any dependencies we have.

@fbartho
Copy link
Member

fbartho commented Mar 16, 2022

Also It’s possible our @types/node expression should be >= node 10 to express that we only rely on features introduced in node 10, rather than a carat-based (^) expression which implies “only supports semver-compatible node versions” which is actually a stricter implication than what we mean.

@glensc
Copy link
Contributor Author

glensc commented Mar 17, 2022

I need a decision what is the minimum node version to proceed on this pr:

and the decision should be marked with "engine" keyword.

@glensc
Copy link
Contributor Author

glensc commented Mar 17, 2022

If you say:

Please don’t downgrade any dependencies we have.

then the only option I see is to bump the engine to >=14, shall I send PR?

@orta
Copy link
Member

orta commented Mar 17, 2022

14 should be fine, it reflects what we're running on CI

@glensc glensc mentioned this pull request Mar 17, 2022
@glensc
Copy link
Contributor Author

glensc commented Mar 17, 2022

there: #1257

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.

3 participants