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

[💡 Feature]: Check for unsupported "engines" package.json fields #11868

Open
1 task done
colinrotherham opened this issue Dec 14, 2023 · 17 comments
Open
1 task done
Labels
good first pick a reasonable task to start getting familiar with the code base help wanted Issues that are free to take by anyone interested Idea 💡 A new feature idea

Comments

@colinrotherham
Copy link
Contributor

Is your feature request related to a problem?

Follow up to #11823 (comment) creating it as an issue

With frequent Dependabot update PRs it's easy to merge updates that drop support for older Node.js versions

Can checks be added to prevent this?

Describe the solution you'd like.

For example, log output already includes silent warnings from npm ci when updates are incompatible:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@lerna/create@8.0.0',
npm WARN EBADENGINE   required: { node: '>=18.0.0' },
npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'form-data-encoder@4.0.2',
npm WARN EBADENGINE   required: { node: '>= 18' },
npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'got@14.0.0',
npm WARN EBADENGINE   required: { node: '>=20' },
npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'lerna@8.0.0',
npm WARN EBADENGINE   required: { node: '>=18.0.0' },
npm WARN EBADENGINE   current: { node: 'v16.20.2', npm: '8.19.4' }
npm WARN EBADENGINE }

Using npm engine-strict=true all incompatible updates from Dependabot PRs will fail to complete npm ci

Describe alternatives you've considered.

Using a package like installed-check

But it would need "workspaces" support as mentioned in #11823 (comment)

npx --workspaces installed-check --engine-check --engine-no-dev

It throws on node_modules not being in the same place as package.json

Unexpected error: Failed to list installed modules: Non-existing path set: /path/to/project/webdriverio/packages/devtools/node_modules

ErrorWithCause: Failed to list installed modules
   at file:///path/to/project/webdriverio/node_modules/installed-check-core/index.js:52:13

Additional context

It's not currently possible to use engine-strict=true with --omit=dev to check non-development dependencies

Note: Dependabot also uses its own Node.js version must be compatible with engine-strict=true. For example, using a minimum Node.js v20 "engines" field would prevent Dependabot running Node.js v18

Code of Conduct

  • I agree to follow this project's Code of Conduct
@colinrotherham colinrotherham added Needs Triaging ⏳ No one has looked into the issue yet Idea 💡 A new feature idea labels Dec 14, 2023
@christian-bromann christian-bromann added help wanted Issues that are free to take by anyone interested good first pick a reasonable task to start getting familiar with the code base and removed Needs Triaging ⏳ No one has looked into the issue yet labels Dec 14, 2023
@wdio-bot
Copy link
Contributor

Thanks for reporting!

We greatly appreciate any contributions that help resolve the bug. While we understand that active contributors have their own priorities, we kindly request your assistance if you rely on this bug being fixed. We encourage you to take a look at our contribution guidelines or join our friendly Discord development server, where you can ask any questions you may have. Thank you for your support, and cheers!

@voxpelli
Copy link
Contributor

I have released installed-check 9.0.0 which includes workspace support, possibly unblocking this

@colinrotherham
Copy link
Contributor Author

@voxpelli Thanks for releasing the new update

I gave it try but the repo now uses pnpm so needs package.json "workspaces" back again

 WARN  The "workspaces" field in package.json is not supported by pnpm. Create a "pnpm-workspace.yaml" file instead.

But you can try it out on my branch main...colinrotherham:node-engines with the following output:

Output

Errors:

e2e: svelte: Narrower "engines.node" is needed: >=16.0.0
e2e: @babel/plugin-transform-react-jsx-development: Narrower "engines.node" is needed: >=6.9.0
e2e: expect: Narrower "engines.node" is needed: ^14.15.0 || ^16.10.0 || >=18.0.0
e2e: mocha: Narrower "engines.node" is needed: >=14.0.0
e2e: puppeteer-core: Narrower "engines.node" is needed: >=16.0.0
e2e: string-width: Narrower "engines.node" is needed: >=8.0.0
e2e: vite: Narrower "engines.node" is needed: ^14.18.0 || >=16.0.0

Suggestions:

e2e: Combined "engines.node" needs to be narrower: ^16.10.0 || >=18.0.0

@christian-bromann
Copy link
Member

@colinrotherham interesting, what would you suggest? It seems like there are some dependencies that have a wider engine field, e.g. include Node.js v14, and cause some issues here?

@colinrotherham
Copy link
Contributor Author

See what @voxpelli thinks?

You're probably not as worried about packages such as string-width not having a maximum Node.js version

Other than ignoring packages, I'm not too sure whether the "Narrower… is needed" errors can be configured

@voxpelli
Copy link
Contributor

voxpelli commented Apr 4, 2024

Thanks for letting me know aboutpnpm-workspace.yaml, I'll add support for it 👍 voxpelli/read-workspaces#2

I wouldn't say that its an issue that some packages support a wider version than you do – only if they have a narrower support range is it a trouble.

On a side note, you have to update this:

pnpm exec installed-check --engine-check --engine-no-dev

To this:

pnpm exec installed-check --engine-check --ignore-dev

As ignore-dev is also used by the new peer-check in 9.0.0 I decided to rename it and I didn't keep the old flag around

@colinrotherham
Copy link
Contributor Author

Great spot, thanks @voxpelli I've updated main...colinrotherham:node-engines

Adding pnpm support would be brilliant. Watch out for the dependency hoisting settings in .npmrc as hoist-workspace-packages will be enabled by default in v9 but can be configured either way still

Output

Errors:

e2e: svelte: Narrower "engines.node" is needed: >=16.0.0

Suggestions:

e2e: Combined "engines.node" needs to be narrower: >=16.0.0

Worth mentioning that my package.json workaround probably hasn't seen all package.json files

@voxpelli
Copy link
Contributor

voxpelli commented Apr 4, 2024

Released version 9.1.0 which has preliminary support for resolving pnpm workspaces + adds a new output in verbose mode that lists all successful and unsuccessful workspaces. Looks like this:

Skärmavbild 2024-04-04 kl  17 20 44

So if you run installed-check --engine-check --ignore-dev --verbose, then you will be able to see all workspaces it has found and whether it deemed them successful or not. This can be handy for initial setup / verification / debugging.

@colinrotherham
Copy link
Contributor Author

Thanks @voxpelli 🙌

Output from --verbose was showing all the workspaces but we hit an issue following pnpm run setup

Each package now has a git-ignored ./build but is flagged as multiple workspaces with the same name

Output

Unexpected error: must not have multiple workspaces with the same name
package '@wdio/cli-cjs' has conflicts in the following paths:
    packages/wdio-cli/build/cjs
    packages/wdio-cli/src/cjs
package '@wdio/cucumber-framework-cjs' has conflicts in the following paths:
    packages/wdio-cucumber-framework/build/cjs
    packages/wdio-cucumber-framework/src/cjs
package '@wdio/json-reporter-cjs' has conflicts in the following paths:
    packages/wdio-json-reporter/build/cjs
    packages/wdio-json-reporter/src/cjs
package '@wdio/shared-store-service-cjs' has conflicts in the following paths:
    packages/wdio-shared-store-service/build/cjs
    packages/wdio-shared-store-service/src/cjs
package 'webdriver-cjs' has conflicts in the following paths:
    packages/webdriver/build/cjs
    packages/webdriver/src/cjs
package 'webdriverio-cjs' has conflicts in the following paths:
    packages/webdriverio/build/cjs
    packages/webdriverio/src/cjs

Error: must not have multiple workspaces with the same name
package '@wdio/cli-cjs' has conflicts in the following paths:
    packages/wdio-cli/build/cjs
    packages/wdio-cli/src/cjs
package '@wdio/cucumber-framework-cjs' has conflicts in the following paths:
    packages/wdio-cucumber-framework/build/cjs
    packages/wdio-cucumber-framework/src/cjs
package '@wdio/json-reporter-cjs' has conflicts in the following paths:
    packages/wdio-json-reporter/build/cjs
    packages/wdio-json-reporter/src/cjs
package '@wdio/shared-store-service-cjs' has conflicts in the following paths:
    packages/wdio-shared-store-service/build/cjs
    packages/wdio-shared-store-service/src/cjs
package 'webdriver-cjs' has conflicts in the following paths:
    packages/webdriver/build/cjs
    packages/webdriver/src/cjs
package 'webdriverio-cjs' has conflicts in the following paths:
    packages/webdriverio/build/cjs
    packages/webdriverio/src/cjs
    at getError (/path/to/project/webdriverio/node_modules/.pnpm/@npmcli+map-workspaces@3.0.4/node_modules/@npmcli/map-workspaces/lib/index.js:64:24)
    at mapWorkspaces (/path/to/project/webdriverio/node_modules/.pnpm/@npmcli+map-workspaces@3.0.4/node_modules/@npmcli/map-workspaces/lib/index.js:146:11)
    at async readWorkspaces (file:///path/to/project/webdriverio/node_modules/.pnpm/read-workspaces@1.1.1/node_modules/read-workspaces/index.js:80:15)
    at async workspaceLookup (file:///path/to/project/webdriverio/node_modules/.pnpm/list-installed@5.2.1/node_modules/list-installed/lib/lookup.js:47:20)
    at async installedCheck (file:///path/to/project/webdriverio/node_modules/.pnpm/installed-check-core@8.2.1/node_modules/installed-check-core/lib/installed-check.js:30:20)
    at async file:///path/to/project/webdriverio/node_modules/.pnpm/installed-check@9.1.0/node_modules/installed-check/cli.js:111:18

@voxpelli
Copy link
Contributor

voxpelli commented Apr 4, 2024

@colinrotherham It's because you use ** in pnpm-workspace.yaml rather than *, would it work do go with eg. packages/* instead of packages/**?

- 'packages/**'
- 'website/**'
- 'e2e/**'
- 'tests/**'

It probably occurs here because I bolted on the pnpm workspace resolving on top of the npm workspace resolving and only really used @pnpm/workspace.read-manifest from pnpm itself rather than the full on @pnpm/workspace.find-packages, as that latter one would do a lot of duplicated work: https://github.com/voxpelli/read-workspaces/blob/746cc1f0d92cdbbd81062b4f603c61bc9e619967/index.js#L135-L136

@colinrotherham
Copy link
Contributor Author

I went ahead and tried narrowing those globs down. Deeply nested workspaces don't have dependencies

webdriverio/pnpm-lock.yaml

Lines 1400 to 1402 in a46cea2

packages/webdriverio/build/cjs: {}
packages/webdriverio/src/cjs: {}

But now it doesn't find the packages 😬

Presume this is because pnpm only symlinks workspace packages from node_modules/.pnpm/*?

Output

Warnings:

...more...

@wdio/browserstack-service: @wdio/cli: Dependency is not installed. Can't check its requirements
@wdio/cli: @types/node: Dependency is not installed. Can't check its requirements
@wdio/cli: @vitest/snapshot: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/config: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/globals: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/logger: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/protocols: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/types: Dependency is not installed. Can't check its requirements
@wdio/cli: @wdio/utils: Dependency is not installed. Can't check its requirements
@wdio/cli: async-exit-hook: Dependency is not installed. Can't check its requirements
@wdio/cli: chalk: Dependency is not installed. Can't check its requirements
@wdio/cli: chokidar: Dependency is not installed. Can't check its requirements

...more...

Successful workspaces: eslint-plugin-wdio, @wdio/allure-reporter, @wdio/appium-service, @wdio/browser-runner, @wdio/browserstack-service, @wdio/cli, @wdio/concise-reporter, @wdio/config, @wdio/cucumber-framework, @wdio/devtools-service, @wdio/dot-reporter, @wdio/firefox-profile-service, @wdio/globals, @wdio/jasmine-framework, @wdio/json-reporter, @wdio/junit-reporter, @wdio/local-runner, @wdio/logger, @wdio/mocha-framework, @wdio/protocols, @wdio/repl, @wdio/reporter, @wdio/runner, @wdio/sauce-service, @wdio/shared-store-service, @wdio/smoke-test-cjs-service, @wdio/smoke-test-reporter, @wdio/smoke-test-service, @wdio/spec-reporter, @wdio/static-server-service, @wdio/sumologic-reporter, @wdio/testingbot-service, @wdio/types, @wdio/utils, @wdio/webdriver-mock-service, webdriver, webdriverio, bidi, cloudservices, devtools, multiremote, pageobject, standalone, wdio, mocha, vite-vue-example, website, e2e, smoke, root

@voxpelli
Copy link
Contributor

voxpelli commented Apr 4, 2024

I released a fix for symlinks 30 minutes ago, along with some other pnpm quality of life improvements: https://github.com/voxpelli/node-installed-check/releases/tag/v9.1.1

I will download your repository and do some more checking

@voxpelli
Copy link
Contributor

voxpelli commented Apr 4, 2024

After some digging I found pnpm/pnpm#3398 and by adapting the suggestion in pnpm/pnpm#3398 (comment) I could conclude that pnpm is also resolving the /build/ workspaces:

❯ pnpm ls -r --depth -1 --long --parseable | grep '@wdio/cli-cjs'
/Users/pelle/Sites/smallrepos/webdriverio/packages/wdio-cli/build/cjs:@wdio/cli-cjs:PRIVATE
/Users/pelle/Sites/smallrepos/webdriverio/packages/wdio-cli/src/cjs:@wdio/cli-cjs:PRIVATE

But unlike npm it doesn't seem like pnpm allows one to target a workspace by name (npm eg. allows for npm run -w @wdio/cli-cjs foo) and as such doesn't need the workspaces to be uniquely named, which is an unfortunate decision on their behalf as that makes them less compatible with npm.

I will have to ponder on a possible solution. I could probably expose a --workspace-ignore that I forward to the ignore in https://www.npmjs.com/package/@npmcli/map-workspaces and which you could then use like --workspace-ignore='**/build/**'

@voxpelli
Copy link
Contributor

voxpelli commented Apr 5, 2024

If you update to installed-check@9.2.0 then you can now do:

pnpm exec installed-check --engine-check --ignore-dev --workspace-ignore='**/build/**'

It makes the repo work after pnpm run setup. Thanks for helping me harden the tool 🙏

@colinrotherham
Copy link
Contributor Author

That was quick, thanks!

I've updated main...colinrotherham:node-engines

Is this output useful @christian-bromann?

Output

Errors:

@wdio/cucumber-framework: @cucumber/cucumber: Narrower "engines.node" is needed: ^18.0.0 || >=20.0.0
@wdio/json-reporter: @wdio/reporter: Narrower "engines.node" is needed: >=18.0.0
@wdio/json-reporter: @wdio/types: Narrower "engines.node" is needed: >=18.0.0
e2e: react: Narrower "engines.node" is needed: >=0.10.0
e2e: svelte: Narrower "engines.node" is needed: >=16.0.0
smoke: webdriver: Narrower "engines.node" is needed: >=18.0.0
smoke: webdriverio: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/cli: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/config: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/logger: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/repl: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/runner: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/utils: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/globals: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/allure-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/concise-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/dot-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/junit-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/spec-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/sumologic-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/appium-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/browserstack-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/devtools-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/firefox-profile-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/sauce-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/shared-store-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/testingbot-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/local-runner: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/browser-runner: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/cucumber-framework: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/jasmine-framework: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/mocha-framework: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/smoke-test-reporter: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/smoke-test-service: Narrower "engines.node" is needed: >=18.0.0
smoke: @wdio/webdriver-mock-service: Narrower "engines.node" is needed: >=18.0.0

Suggestions:

@wdio/cucumber-framework: Combined "engines.node" needs to be narrower: ^18.0.0 || >=20.0.0
@wdio/json-reporter: Combined "engines.node" needs to be narrower: >=18.0.0
e2e: Combined "engines.node" needs to be narrower: >=16.0.0
smoke: Combined "engines.node" needs to be narrower: >=18.0.0

@voxpelli
Copy link
Contributor

voxpelli commented Apr 5, 2024

I added a --fix in installed-check@9.3.0 as it makes more sense now with big monorepos like this. So you can now at least fix these easily 🙂

@colinrotherham
Copy link
Contributor Author

Thanks @voxpelli, PR opened 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first pick a reasonable task to start getting familiar with the code base help wanted Issues that are free to take by anyone interested Idea 💡 A new feature idea
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants