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

Pin Node.JS version in GitHub Actions/Netlify to v18.18.2 using NVM (removes volta) #5097

Closed

Conversation

aloisklink
Copy link
Member

@aloisklink aloisklink commented Dec 3, 2023

📑 Summary

Pin our Node.JS version to v18.18.2 using NVM.

This is important as our Netlify builds have recently stopped working, see https://app.netlify.com/sites/mermaid-js/deploys/656b6ffb3872da00082fda6f.

This is because Node.JS v18.19.0 came out on 2023-11-29 and Netlify recently switched to using it. This version of Node.JS is incompatible with ts-node-esm, which is used for our build script, see TypeStrong/ts-node#2094.

Because of this, we need to tell Netlify to use Node.JS v18.18.2, which can be done by setting a .nvmrc file, see https://docs.netlify.com/configure-builds/manage-dependencies/#node-js-and-javascript.

I've also told GitHub Actions to use Node.JS v18.18.2, since although they're still cached on that version, they'll soon upgrade to v18.19.0.

📏 Design Decisions

Why am I switching from volta to nvm

Currently, we're using volta to manage our Node.JS version. Unfortunately, Volta isn't as compatible with CI (netlify/GitHub Actions), so I've moved to NVM.

netlify build log error

For posterity, the error message from netlify's build logs is:

5:57:39 PM: Now using node v18.19.0 (npm v10.2.3)
[...REMOVED UNNECESSARY LINES...]
5:57:50 PM: . prepare: > ts-node-esm --transpileOnly .vite/build.ts
5:57:51 PM: Failed during stage "Install dependencies": dependency_installation script returned non-zero exit code: 1
5:57:51 PM: . prepare: TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /opt/build/repo/.vite/build.ts
5:57:51 PM: . prepare:     at new NodeError (node:internal/errors:405:5)
5:57:51 PM: . prepare:     at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:136:11)
5:57:51 PM: . prepare:     at defaultGetFormat (node:internal/modules/esm/get_format:182:36)
5:57:51 PM: . prepare:     at defaultLoad (node:internal/modules/esm/load:101:20)
5:57:51 PM: . prepare:     at nextLoad (node:internal/modules/esm/hooks:864:28)
5:57:51 PM: . prepare:     at load (/opt/build/repo/node_modules/.pnpm/ts-node@10.9.1_@types+node@18.17.5_typescript@5.1.6/node_modules/ts-node/dist/child/child-loader.js:19:122)
5:57:51 PM: . prepare:     at nextLoad (node:internal/modules/esm/hooks:864:28)
5:57:51 PM: . prepare:     at Hooks.load (node:internal/modules/esm/hooks:447:26)
5:57:51 PM: . prepare:     at MessagePort.handleMessage (node:internal/modules/esm/worker:196:24)
5:57:51 PM: . prepare:     at [nodejs.internal.kHybridDispatch] (node:internal/event_target:786:20) {
5:57:51 PM: . prepare:   code: "ERR_UNKNOWN_FILE_EXTENSION"

📋 Tasks

Make sure you

nvm has better compatibility with GitHub Actions and Netlify.
I've excluded other CI files since they use a build matrix
(even if just a single value), so it's not as easy to use replace with
this `.nvmrc` file.
Node v18.19.0 breaks `ts-node-esm`, so `pnpm install` won't work.

See: TypeStrong/ts-node#2094
Copy link

netlify bot commented Dec 3, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 81a9324
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/656cfe3eac34370008892a18
😎 Deploy Preview https://deploy-preview-5097--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Other Not an enhancement or a bug label Dec 3, 2023
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Merging #5097 (81a9324) into develop (f81e4d4) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5097   +/-   ##
========================================
  Coverage    79.38%   79.38%           
========================================
  Files          166      166           
  Lines        13871    13871           
  Branches       705      705           
========================================
  Hits         11012    11012           
  Misses        2707     2707           
  Partials       152      152           
Flag Coverage Δ
e2e 85.20% <ø> (ø)
unit 43.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@aloisklink
Copy link
Member Author

aloisklink commented Dec 3, 2023

It looks like we can't merge this, since GitHub expects the CI runs to be called 18.x instead of 18.18.2, @sidharthv96, @knsv, any ideas?

image

Maybe instead of using matrix.node-version, we can just use the same .nvmrc file. It would mean that we can only test on a single version of Node.JS instead of multiple versions, but that's what we're doing anyway (plus, Mermaid is a browser library, not a Node.JS software):

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

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

Can't we use pnpm itself to manage node? https://pnpm.io/cli/env
That would mean one less tool.

Netlify is currently working as I set the NODE_VERSION environment variable in netlify.

@aloisklink
Copy link
Member Author

aloisklink commented Dec 4, 2023

Can't we use pnpm itself to manage node? https://pnpm.io/cli/env

Yep, we could, but then what tool would we use to manage pnpm? I believe we're currently using corepack (Node.JS's built-in version manager for yarn/pnpm) to automatically install/update pnpm:

"packageManager": "pnpm@8.11.0",

It's also not as nice as just using the following in GitHub Actions:

  - name: Setup Node.js
    uses: actions/setup-node@v4
    with:
      cache: pnpm
      node-version-file: '.nvmrc'

Netlify is currently working as I set the NODE_VERSION environment variable in netlify.

🚀 Awesome! In that case, this PR isn't urgent (yet).

Maybe a better way of fixing this issue would be to switch to using https://www.npmjs.com/package/tsx instead of ts-node-esm. Or we could try ts-node-esm's v11 beta release too. I can have a look this evening.

@sidharthv96
Copy link
Member

I remember having some problems after switching to tsx. Don't recall the exact issue. Maybe it's fixed now.

@aloisklink
Copy link
Member Author

I remember having some problems after switching to tsx. Don't recall the exact issue. Maybe it's fixed now.

I've made a PR for replacing ts-node-esm with tsx that will also close this PR and it all seems to work, see #5104. If you're talking about 7533728, I think it's just because we forgot to remove the --transpileOnly flag, but 🤷, it's entirely possible that ESBuild was missing some TypeScript features that we use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: DevOps Type: Other Not an enhancement or a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants