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

Fix/remove npm engine specifier #168

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

lukekaalim
Copy link
Contributor

The package.json includes an "engines" field declaring a requirement for node 16.15.1.

If you start an empty project with the following package.json:

{
  "name": "testproj",
  "version": "1.0.0",
  "dependencies": {
    "@bitmovin/player-integration-yospace": "^2.3.0"
  }
}

running node 18, you will receive the following warning:

lkaalim@N1003643 testproj % npm i @bitmovin/player-integration-yospace                                           
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@bitmovin/[email protected]',
npm WARN EBADENGINE   required: { node: '16.15.1' },
npm WARN EBADENGINE   current: { node: 'v16.14.0', npm: '9.8.1' }
npm WARN EBADENGINE }

As the @bitmovin/player-integration-yospace package is mostly intended to run on browsers and video-playing-environments, theres no actual requirement for consumers of this package to have a specific version of node. In fact, if you have any version of node apart from 16.15.1 specifically, npm will report that warning.

If using yarn, the error is more fatal:

lkaalim@N1003643 testproj % yarn
yarn install v1.22.21
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
error @bitmovin/[email protected]: The engine "node" is incompatible with this module. Expected version "16.15.1". Got "18.19.0"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

When installing this own package for development purposes and during test runs in CI, you also see the same warning as this project itself uses node 18 as defined in it's .nvmrc:

v18.13.0

The engines field isn't relevant for this project, as the nvmrc file determine what version of node someone developing this package should use, and the version of node that a consumer happens to use to install the project has no relevance on the actual runtime expected from this package.

If there is somehow a requirement for clients to use specific versions of node, the engine field should be updated to be either:

  • "node": "~16.15.1" (for node 16 specifically)
  • "node": ">16.15.1" (for node 16 and above)

The .nvmrc file determines what version of node is used for CI - the npm engine field determines what version of node consumers of this package need. As most clients will be web browsers, the version of node they have installed will be based on what build tooling they happen to have, which isn't really relevant for compatibility.
@CLAassistant
Copy link

CLAassistant commented Feb 9, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Luke Kaalim seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@dweinber dweinber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lukekaalim, a valid change 👍

Please see my suggestion regarding the Changlog to adhere to the Changelog format. After that we're good to merge 🙂

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel Weinberger <[email protected]>
@dweinber dweinber merged commit df06936 into bitmovin:develop Feb 14, 2024
1 of 2 checks passed
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