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

Set Node to v20 #6055

Merged
merged 5 commits into from Oct 31, 2023
Merged

Set Node to v20 #6055

merged 5 commits into from Oct 31, 2023

Conversation

travellingprog
Copy link
Contributor

Issue

Closes #5984

(Although a wiki update will still be required once this is merged)

Description

I'm new to this project and decided to take on this issue because I ran into it when I first setup this project. Sorry ahead of time for this long message, but I figure you'd appreciate seeing my reasoning.

First, I made .npmrc file changes, adding in the setting use-node-version. This will make PNPM use Node v20.9.0 when running any package.json script. PNPM will actually download that version of Node if it does not detect it on the developer's machine. I tried to use a less specific version (e.g use-node-version=20), but then PNPM complained that this was not a correct format. An alternative approach would be to use the popular tool [NVM}(https://github.com/nvm-sh/nvm) along with adding new .nvmrc files, which does allow only specifying a major version. Also, I don't know if this is going to require changes or not in your CI environment.

Second, the issue we were seeing with running the project with Node 20 was simply caused by the fact that ts-node has not been updated since July 2022, and therefore it does not have a targeted configuration for running with ESM mode in Node 20. Indeed, there is a long-standing open issue about this.

In the long issue thread, you will see that most people tackle this issue by running their script with node --experimental-loader=ts-node/esm (or node --loader=ts-node/esm, which is the same thing). However, this approach is discouraged in the Node 20 CLI documentation. It also has the unpleasant side-effect of displaying a long warning in the terminal, which can only be disabled by turning off all warnings from Node. The good thing is that the warning message output the code that is now in the new file web/scripts/register-tsNodeESM.js, a module resolution hook, which allows us to use the preferred --import flag.

Note that there is a ts-node PR which will add a similar hook to ts-node, hopefully in an upcoming release. Also, after no releases in a long time, ts-node had a beta released last week for a new major version. So this script I added may not be needed for too long, but in the meanwhile I think it's good that we upgrade to the latest Node LTS.

Double check

  • I have run pnpx prettier --write . and poetry run format to format my changes.

@github-actions github-actions bot added frontend 🎨 dependencies Pull requests that update a dependency file labels Oct 26, 2023
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Looking at this PR it is missing an update to the CI files which are under .github/workflows

To that point I also think it would be preferable to use the engine field in package.json over .npmrc files. An alternative would be to define a top level .node-version file. Our CI could be made to consume both of these to ensure they are always in sync.

@travellingprog
Copy link
Contributor Author

@VIKTORVAV99 I updated the CI files and decided to go with updating both the package.json file and also adding in .node-version files, so I can use it with fnm.

I have two possible suggestions now:

  • I noticed in the CI files that everytime we setup Node, we follow that up with an actions/cache step. However, in reading the docs for actions/setup-node, I noticed that it can make use of cache within in, and apparently its use of cache follows best practices. You can find that documented here. The action also re-outputs cache-hit. It would make our workflow code simpler.
  • I noticed we have 4 steps that could be put in a reusable workflow: Setup PNPM, Setup Node, Cache dependencies and then install dependencies. We could call that workflow setup-web, for example.

Let me know what you think about that

@VIKTORVAV99
Copy link
Member

The built in cache only caches the pnpm cache which is slower than what we do here (cache the entire node folder and skip install) this has been tested and the current method resulted in the lowest CI time.

You can try and change how the workflows work if you wish but I'm not sure they work that way. Was a while since I took a look at them though.

@travellingprog
Copy link
Contributor Author

@VIKTORVAV99 Ok, understood. I think my second suggestion (creating a setup-web reusable workflow) should probably be its own PR anyways, and should probably be done with someone with higher-level access than me, who can test it out multiple times easily.

The CI workflow failed on October 27 because two jobs failed. The Cypress job failed because the cypress-io/github-action action was set to run with the Node version on the runner, which by default seems to be Node 18. So I added the usual "setup Node + get dependencies caches + install dependencies if needed" steps we have in the other workflow files, and disabled the install done internally by the Cypress job. I figure its better for us to have a consistent approach across our files.

The Deploy Preview job also failed, with the error message "secret CLOUDFLARE_API_TOKEN is required, but not provided while calling". I don't know why the secret would not have been present at the CI run, maybe some Github permission setting that needs to be changed? Could you please look into it if it occurs again?

@travellingprog
Copy link
Contributor Author

travellingprog commented Oct 31, 2023

@VIKTORVAV99 Yesterday, the Cypress CI job failed with the following error:

Error: The cypress npm package is installed, but the Cypress binary is missing.

We expected the binary to be installed here: /home/runner/.cache/Cypress/12.9.0/Cypress/Cypress

Reasons it may be missing:

- You're caching 'node_modules' but are not caching this path: /home/runner/.cache/Cypress
- You ran 'npm install' at an earlier build step but did not persist: /home/runner/.cache/Cypress

Properly caching the binary will fix this error and avoid downloading and unzipping Cypress.

Alternatively, you can run 'cypress install' to download the binary again.

https://on.cypress.io/not-installed-ci-error

Basically, the installation of Cypress is one of those cases where caching node_modules is not enough. I looked at the recommendations from Cypress and also their examples to add more steps to the Cypress jobs. On the positive side, if all these changes work, they should make the Cypress jobs noticeably faster than before this PR, once the cache has been set.

I also added a comment on the register-tsNodeESM.js file, something I had meant to do but kept forgetting.

@travellingprog
Copy link
Contributor Author

@VIKTORVAV99 I believe this PR should be okay to merge now. The only CI job that is failing is the Preview one, because my fork branch cannot have access to Github Secrets.

@VIKTORVAV99 VIKTORVAV99 self-requested a review October 31, 2023 22:24
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking care of this!

@VIKTORVAV99 VIKTORVAV99 merged commit f9c7dae into electricitymaps:master Oct 31, 2023
18 of 19 checks passed
mathilde-daugy pushed a commit that referenced this pull request Nov 2, 2023
* fix: set front-end to Node v20, fix use of ts-node

* fix: Cypress CI workflow now uses required Node version and dependencies cache

* fix: Add install=false for cypress-io/github-action

* fix: in CI workflow, install and cache Cypress binary

* docs: add JSDoc description to register-tsNodeESM.js file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with Node.js 20
2 participants