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

chore: Upgrade to newest Node LTS version #993

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

munen
Copy link
Collaborator

@munen munen commented Sep 20, 2024

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
    • Note: Atm, it works only with the --openssl-legacy-provider workaround. There's inline comments for more info.
  • yarn run build
  • yarn prettier-eslint --write
  • yarn eslint

CI needs to work

  • Upgrade Docker image for CI
  • CI should be green

Manual integration test

  • /sample still works
  • Logging into dropbox still works
  • Logging into gitlab still works
  • Logging into webdav still works

@@ -11,7 +11,7 @@ setup:

.PHONY: run
run: setup
yarn start
NODE_OPTIONS=--openssl-legacy-provider yarn start
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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..

Copy link
Collaborator

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.
@schoettl
Copy link
Collaborator

At this point, for yarn build, I should probably upgrade eslint to allow date-fns use of newer language features (classProperties). But react-scripts seem to depend on the old eslint...

Next is to upgrade eslint* and react* and fix all errors at once...

@munen
Copy link
Collaborator Author

munen commented Sep 23, 2024

"In between" kudos for the progress on this PR 💯

@schoettl
Copy link
Collaborator

I now get this success message for yarn start
image

But the browser page stays blank with

Uncaught ReferenceError: process is not defined

It's because of the upgrade to react-scripts v5 [1]. For some NodeJS globals like crypto, process, etc., react-scripts used to generate polyfills. In previous versions, process.env.MY_ENV_VAR was apparently poly-filled and inlined at compile-time. We use that in some places, e.g. src/components/SyncServiceSignIn/index.js.

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

@schoettl
Copy link
Collaborator

Oh, it seems to be related with dotenv which I removed because I didn't understand how it's used....

@schoettl
Copy link
Collaborator

No, it's still the upgrade to react-scripts v5.
facebook/create-react-app#12354

When I downgrade back to v4 I'd also need to downgrade eslint (from 9 to 7) :-(

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.

Upgrade node and dependencies?
2 participants