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

Can't use shaka-player in projects that use node < 14 #5243

Closed
caridley opened this issue May 22, 2023 · 11 comments · Fixed by #5253
Closed

Can't use shaka-player in projects that use node < 14 #5243

caridley opened this issue May 22, 2023 · 11 comments · Fixed by #5253
Assignees
Labels
component: build system The issue involves the build system of Shaka Player status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@caridley
Copy link
Contributor

Have you read the FAQ and checked for duplicate open issues?
yes

What version of Shaka Player are you using?
4.3.4

Can you reproduce the issue with our latest release version?
yes

Can you reproduce the issue with the latest code from main?
yes

Are you using the demo app or your own custom app?
custom app

If custom app, can you reproduce the issue using our demo app?
Not applicable

What browser and OS are you using?
No relevant

For embedded devices (smart TVs, etc.), what model and firmware version are you using?
Not relevant

What are the manifest and license server URIs?
Not relevant

What configuration are you using? What is the output of player.getConfiguration()?
Not relevant

What did you do?
Tried to use shaka version 4.3.4 in a project that is currently using node 10.19

What did you expect to happen?
Expected yarn install to succeeed

What actually happened?
Yarn install of our project that uses shaka-player fail on shaka-player because shaka-player package.json has engines.node ">= 14"

node 14 is required when building the shaka-player library, but is not required when using it.

The engines.node should not be present in the package.json in the distribution

@caridley caridley added the type: bug Something isn't working correctly label May 22, 2023
@github-actions github-actions bot added this to the v4.4 milestone May 22, 2023
@alexandercerutti
Copy link
Contributor

alexandercerutti commented May 25, 2023

I think to have reported the same thing some months ago here: #4721. Is this maybe a duplicate?

We ended up upgrading Node from 12 to 14, by the way.

Anyway, Node 10 was LTS until like 2 years ago.
Now Node 14 is now at the "end of life" since the end of April 2023.

@joeyparrish
Copy link
Member

Yarn install of our project that uses shaka-player fail on shaka-player because shaka-player package.json has engines.node ">= 14"

node 14 is required when building the shaka-player library, but is not required when using it.

The engines.node should not be present in the package.json in the distribution

This is because some of our build or test dependencies require node >= 14. Is there a way to specify engines.node separately for a dev install?

@alexandercerutti
Copy link
Contributor

alexandercerutti commented May 26, 2023

@joeyparrish what about something like NVM (Node Version Manager) with a .nvmrc or pnpm (which is a package manager, but also includes such NVM feature)?

I mean, that would not be strict, so you'd have to document it for development.

@joeyparrish
Copy link
Member

Developers can definitely use NVM if they wish, but I can't imagine requiring it at a project level.

@alexandercerutti
Copy link
Contributor

alexandercerutti commented May 26, 2023

I meant setting NVM on Shaka through an .npmrc.

It is pretty common to have it on any kind of project. It is not one of those dependencies that should belong only to some specific kinds of projects (e.g. final products).

@joeyparrish
Copy link
Member

Sorry, I'm not familiar with setting NVM through npmrc, and I didn't find anything obvious from a Google search. Can you give an example or link I could learn from?

@alexandercerutti
Copy link
Contributor

alexandercerutti commented May 26, 2023

@joeyparrish sure! Here it is:

https://github.com/nvm-sh/nvm
https://github.com/nvm-sh/nvm#nvmrc

It is easy as just hardcoding the node version inside the text file and installing NVM on your system.

Anyway, another possibility would to introduce a programmatic check like this below, but maybe by hardcoding the required version instead of reading it from package.json.

import semver from 'semver';
import { engines } from './package';

const version = engines.node;

if (!semver.satisfies(process.version, version)) {
  console.log(`Required node version ${version} not satisfied with current version ${process.version}.`);
  process.exit(1);
}

@joeyparrish
Copy link
Member

Ah, you mean nvmrc, not npmrc. I see. Sure, that's no big deal. Send a PR if you like.

However, it doesn't seem to address the original issue, which states:

Yarn install of our project that uses shaka-player fail on shaka-player because shaka-player package.json has engines.node ">= 14"

I could be wrong, but I think so long as we have dependencies that require node >= 14, it's appropriate to have that same requirement in shaka-player itself.

But after searching through actual dependency and transitive dependency, I'm finding things more nuanced. The strictest engine setting in our dependencies seems to be from some of the eslint packages:

  "engines": {
    "node": "^12.22.0 || ^14.17.0 || >=16.0.0"
  },

In fact, the addition in our package.json only came about a year ago, in PR #4219, where it is noted that our dev deps generally require 12.

@caridley, what's your environment? Can you satisfy node >= 12? If so, I suggest someone could change our minimum to more closely match what eslint requires. (I'm guessing 12.22 and 14.17 are final releases when those branches went EOL.)

@alexandercerutti
Copy link
Contributor

alexandercerutti commented May 26, 2023

Ah, you mean nvmrc, not npmrc. I see. Sure, that's no big deal. Send a PR if you like.

Sorry @joeyparrish, I wrote it wrong the second time ahah
Actually .npmrc exists but it is for other purposes!

However, it doesn't seem to address the original issue, which states:

I think that having a .nvmrc would allow shaka to not have engines.node hardcoded, because .nvmrc is evaluated only on development and when developers run nvm use.

Also, I think the original purpose for engines.node is made for production in libraries, not for development. That's kind of a shame. I'd honestly prefer something tailored for development.

Anyway, IMHO, I'd not bring the engines.node down: Node 14 and lower are now out of LTS, so it should be the burden of projects that use libraries to keep up. Otherwise, every library out there would be still supporting Node 0.10, I guess.

@caridley
Copy link
Contributor Author

There is some good discussion about modifying package.json for publishing here

https://stackoverflow.com/questions/40942495/clean-package-json-before-packing

And a reference to the following toold

https://www.npmjs.com/package/clean-package

@joeyparrish
Copy link
Member

I'm going to implement clean-package and try to get it into our next releases.

joeyparrish added a commit to joeyparrish/shaka-player that referenced this issue May 30, 2023
Before packaging for NPM, remove the "engines" section of package.json
to avoid unnecessary restrictions on node versions for projects that
simply depend on Shaka Player.

The "engines" section and its restrictions still make sense for Shaka
Player development.

Closes shaka-project#5243
@joeyparrish joeyparrish self-assigned this May 30, 2023
@joeyparrish joeyparrish added the component: build system The issue involves the build system of Shaka Player label May 30, 2023
joeyparrish added a commit that referenced this issue Jun 1, 2023
Before packaging for NPM, remove the "engines" section of package.json
to avoid unnecessary restrictions on node versions for projects that
simply depend on Shaka Player, but don't need to rebuild it. This is
accomplished with the clean-package tool:
https://github.com/roydukkey/clean-package

The "engines" section and its restrictions still make sense for Shaka
Player development, so it will not be removed from package.json in the
repo.

Closes #5243
JulianDomingo pushed a commit that referenced this issue Jun 20, 2023
Before packaging for NPM, remove the "engines" section of package.json
to avoid unnecessary restrictions on node versions for projects that
simply depend on Shaka Player, but don't need to rebuild it. This is
accomplished with the clean-package tool:
https://github.com/roydukkey/clean-package

The "engines" section and its restrictions still make sense for Shaka
Player development, so it will not be removed from package.json in the
repo.

Closes #5243
JulianDomingo pushed a commit that referenced this issue Jun 20, 2023
Before packaging for NPM, remove the "engines" section of package.json
to avoid unnecessary restrictions on node versions for projects that
simply depend on Shaka Player, but don't need to rebuild it. This is
accomplished with the clean-package tool:
https://github.com/roydukkey/clean-package

The "engines" section and its restrictions still make sense for Shaka
Player development, so it will not be removed from package.json in the
repo.

Closes #5243
JulianDomingo pushed a commit that referenced this issue Jun 20, 2023
🤖 I have created a release *beep* *boop*
---


##
[4.2.11](v4.2.10...v4.2.11)
(2023-06-20)


### Bug Fixes

* **Demo:** Fix deployment of codem-isoboxer in the Demo
([#5257](#5257))
([23d48d4](23d48d4))
* **Demo:** Fix error link width to avoid overlap with close button
([#5309](#5309))
([a6f980d](a6f980d))
* Fix error when network status changes on src= playbacks
([#5305](#5305))
([ce354ba](ce354ba))
* **HLS:** Avoid "Possible encoding problem detected!" when is a preload
reference
([#5332](#5332))
([763ae6a](763ae6a))
* **HLS:** Avoid HLS resync when there is a gap in the stream
([#5284](#5284))
([256cf20](256cf20))
* **HLS:** Avoid variable substitution if no variables
([#5269](#5269))
([b549b60](b549b60))
* **HLS:** Parse EXT-X-PART-INF as media playlist tag
([#5311](#5311))
([d78c080](d78c080))
* **HLS:** Skip EXT-X-PRELOAD-HINT without full byterange info
([#5294](#5294))
([e462711](e462711))
* media source object URL revocation
([#5214](#5214))
([80ce378](80ce378))
* Ship to NPM without node version restrictions
([#5253](#5253))
([41c1ace](41c1ace)),
closes
[#5243](#5243)
* unnecessary parsing of in-band pssh when pssh is in the manifest
([#5198](#5198))
([889cc68](889cc68))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
JulianDomingo pushed a commit that referenced this issue Jun 21, 2023
🤖 I have created a release *beep* *boop*
---


##
[4.3.7](v4.3.6...v4.3.7)
(2023-06-21)


### Bug Fixes

* CEA 608 captions not work with H.265 video streams
([#5252](#5252))
([b08bb41](b08bb41)),
closes
[#5251](#5251)
* **Demo:** Fix deployment of codem-isoboxer in the Demo
([#5257](#5257))
([7e2903a](7e2903a))
* **demo:** Fix deployment of v4.3.x on appspot
([ccf5e2e](ccf5e2e))
* **Demo:** Fix error link width to avoid overlap with close button
([#5309](#5309))
([f575dab](f575dab))
* Fix error when network status changes on src= playbacks
([#5305](#5305))
([cf683f5](cf683f5))
* **HLS:** Avoid "Possible encoding problem detected!" when is a preload
reference
([#5332](#5332))
([9ce8cc0](9ce8cc0))
* **HLS:** Avoid HLS resync when there is a gap in the stream
([#5284](#5284))
([679dbae](679dbae))
* **HLS:** Avoid variable substitution if no variables
([#5269](#5269))
([49afa92](49afa92))
* **HLS:** Fix HLS seekRange for live streams
([#5263](#5263))
([03df9cb](03df9cb))
* **HLS:** Fix seekRange for EVENT playlist not using
EXT-X-PLAYLIST-TYPE
([#5220](#5220))
([562831b](562831b))
* **HLS:** Parse EXT-X-PART-INF as media playlist tag
([#5311](#5311))
([f6210ee](f6210ee))
* **HLS:** Skip EXT-X-PRELOAD-HINT without full byterange info
([#5294](#5294))
([9e193e2](9e193e2))
* media source object URL revocation
([#5214](#5214))
([1a89daa](1a89daa))
* Ship to NPM without node version restrictions
([#5253](#5253))
([ca096a8](ca096a8)),
closes
[#5243](#5243)
* unnecessary parsing of in-band pssh when pssh is in the manifest
([#5198](#5198))
([8d6494d](8d6494d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: build system The issue involves the build system of Shaka Player status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants