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

Update npm dependencies #2098

Conversation

DivineDominion
Copy link

@DivineDominion DivineDominion commented Nov 8, 2023

Addresses all low hanging fruit for #2095, bumping versions that work well together.

I ran a couple of clean npm install and also npm update and npm audit changes.

Node v17+ also wouldn't allow react-scripts build etc. and an apparent fix is to disable the security check:
https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported

I did that in the package scripts section, too. Thing is: react-scripts probably need to go anyway because there's effectively no maintenance and dependency updates have been unaddressed for years. react.dev recomment Vite nowadays instead of "Create React App", so supposedly, Vite is going to be the future? https://cathalmacdonnacha.com/migrating-from-create-react-app-cra-to-vite -- but that's not for today :)

Oh, npm run build and npm run start work fine 👍

@s-martin s-martin added enhancement ext dependency future3 Relates to future3 development labels Nov 8, 2023
src/webapp/package.json Show resolved Hide resolved
Comment on lines +28 to +31
"start": "react-scripts --openssl-legacy-provider start",
"build": "react-scripts --openssl-legacy-provider build",
"test": "react-scripts --openssl-legacy-provider test",
"eject": "react-scripts --openssl-legacy-provider eject"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? (Didn't do the research myself :-))

Copy link
Author

Choose a reason for hiding this comment

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

Node v17+ also wouldn't allow react-scripts build etc. and an apparent fix is to disable the security check:
https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported

Without this, it just crashes.

Alternative: downgrade Node.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think v17 doesn't bring us further. The LTS for v16 just has come to an end, the recommended version ist now v20. Have you tried this? Or at least the previous LTS v18?

This is independenz from the flag you have added here. I would have to double down on it in order to find a resolution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that you are not actually changing the Node version with your commit. My comment remains valid though it's irrelevant for the PR :-)

Copy link
Author

Choose a reason for hiding this comment

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

I've used node v21.1 for the process. That's the default that was installed, btw, when grabbing future3/develop (or my branch) and building from scratch.

Can bump node to 20, too, if you like.

(I'm not even sure how far back it's technically possible to go, maybe on old hardware or OS's.)

@DivineDominion
Copy link
Author

@pabera Of course on another fresh install, things don't go as smooth today!

node:internal/modules/cjs/loader:452
      throw err;
      ^

Error: Cannot find module '/home/pi/RPi-Jukebox-RFID/src/webapp/node_modules/hoopy/index.js'. Please verify that the package.json has a valid "main" entry
    at tryPackage (node:internal/modules/cjs/loader:444:19)
    at Module._findPath (node:internal/modules/cjs/loader:715:18)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1130:27)
    at Module._load (node:internal/modules/cjs/loader:985:27)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at Object.<anonymous> (/home/pi/RPi-Jukebox-RFID/src/webapp/node_modules/bfj/src/match.js:6:15)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32) {
  code: 'MODULE_NOT_FOUND',
  path: '/home/pi/RPi-Jukebox-RFID/src/webapp/node_modules/hoopy/package.json',
  requestPath: 'hoopy'
}

Node.js v21.1.0

So no need to test anything on your end just yet. I'll play around with this until it's fixed a.t.m.

@pabera
Copy link
Collaborator

pabera commented Nov 10, 2023

No worries.

@DivineDominion
Copy link
Author

The error was a fluke -- a user problem :) I didn't notice that the package-lock.json was locally overwritten with the old values. Resetting that file to this branch/PR's content worked. Tried with a couple of rm -rf node_modules and npm install and ./run_rebuild.sh. So it's good to go IMHO

@pabera
Copy link
Collaborator

pabera commented Nov 11, 2023

I had a look at this PR. There are a few major version updates which can be risky. Specifically i18next v23 is a breaking version update and we can't guarantee that everything works smoothly.

If you don't mind, I would like to stick to the same major versions for now and only update minor versions. I have a new branch/PR open where I am updating dependencies for all aspects (node 20, python venv, bookworm)

#2103

DivineDominion referenced this pull request Nov 13, 2023
* add --break-system-packages option to pip3 install

as required for bookworm, see #2050 (comment)

* install libasound2-dev

to fix installation of pyalsaaudio via pip3

* install NodeJS and npm via official recommendation

see https://github.com/nodesource/distributions

* configure break-system-packages option globally

see discussion in #2100 (review)

* allow nginx (and others) to access /home/pi
@DivineDominion
Copy link
Author

Alright, thanks for looking into this!

@DivineDominion DivineDominion deleted the update-npm-dependencies branch November 13, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ext dependency future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants