-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
chore: Upgrade to newest Node LTS version #993
base: master
Are you sure you want to change the base?
Conversation
@@ -11,7 +11,7 @@ setup: | |||
|
|||
.PHONY: run | |||
run: setup | |||
yarn start | |||
NODE_OPTIONS=--openssl-legacy-provider yarn start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment, this is a workaround. Without that flag, OpenSSL has errors. On the one hand, we don't really need harder OpenSSL security defaults (I think), because we have no backend. However, introducing workarounds for legacy options is probably also not the best idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has been introduced since node v17. So, if we cannot fix it and it is a hard problem, we could try upgrading to v16 first. However, v16 is not maintained anymore, as well. So that's not the greatest of all options..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also had this in my flake.nix. Let's see how it works out after all package upgrades.
... because I don't understand how it's used. And node 20.6 has the option --env-file which might be a replacement.
To remove this error message I added the suggested dependency. It would probably better to get rid of the unmaintained create-react-app but it's not a dependency. The build error was: One of your dependencies, babel-preset-react-app, is importing the "@babel/plugin-proposal-private-property-in-object" package without declaring it in its dependencies. This is currently working because "@babel/plugin-proposal-private-property-in-object" is already in your node_modules folder for unrelated reasons, but it may break at any time. babel-preset-react-app is part of the create-react-app project, which is not maintianed anymore. It is thus unlikely that this bug will ever be fixed. Add "@babel/plugin-proposal-private-property-in-object" to your devDependencies to work around this error. This will make this message go away.
At this point, for Next is to upgrade eslint* and react* and fix all errors at once... |
"In between" kudos for the progress on this PR 💯 |
See facebook/create-react-app#11770 Compressed and optimized SVGs using https://github.com/svg/svgo
... for now because newer versions have breaking changes
I now get this success message for But the browser page stays blank with
It's because of the upgrade to react-scripts v5 [1]. For some NodeJS globals like For most globals I've found replacements (commit a0e7a68) but not yet for process. [1] https://medium.com/@runawaytrike/upgrading-react-scripts-to-v5-for-idiots-16ba48d01c20 |
Oh, it seems to be related with |
No, it's still the upgrade to react-scripts v5. When I downgrade back to v4 I'd also need to downgrade eslint (from 9 to 7) :-( |
Closes: #992
As for the specific NodeJS version used, I'm completely open. I went with v20.17.0 in this first spike, because that's the latest LTS version as far as I can see. If, for any reason, that's not a good choice (for example on Nix), let's use a different new version.
TODOs until this PR can be merged:
CLI tooling needs to work
These tasks should run:
rm -rf node_modules; yarn install
yarn test
yarn run start
--openssl-legacy-provider
workaround. There's inline comments for more info.yarn run build
yarn prettier-eslint --write
yarn eslint
CI needs to work
Manual integration test