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 node versions #617

Open
josephjclark opened this issue Mar 1, 2024 · 6 comments
Open

Update node versions #617

josephjclark opened this issue Mar 1, 2024 · 6 comments
Assignees

Comments

@josephjclark
Copy link
Collaborator

josephjclark commented Mar 1, 2024

  • Ensure everything works in node 18.19
  • Ensure everything works in 20.latest
  • Ensure everything works in 21.x

Ideally we should just work across all vaguely modern node versions.

18.19 did something I think to break the loader vm arg, which causes all sorts of problems.

Remember to update the circle config so that CI is also running in the latest version. I'd actually love to run in a matrix with 18,20,22 (at least for a little while)

  • Ava tests fail
  • the describe-package esbuild script fails

Some constraints on the work:

  • The fix should be minimal - ideally this is just a tweak to a couple of config files
  • I am hoping that we can continue to test in memory in a single command, rather than compiling tests into .js files and leaving them on disk
@github-project-automation github-project-automation bot moved this to New Issues in v2 Mar 1, 2024
@christad92 christad92 moved this from New Issues to Backlog in v2 Mar 4, 2024
@Akshanshkaushal
Copy link

hi @josephjclark @christad92 To better understand the codebase, I want to solve this issue. Could you please assign me this?

@josephjclark
Copy link
Collaborator Author

Sorry @Akshanshkaushal this is one I want to take on myself

@josephjclark
Copy link
Collaborator Author

josephjclark commented Jul 18, 2024

So the problem we have is that node 20 changes the way loaders work, which breaks our ava config. This will affect adaptors as well - see OpenFn/adaptors#187

I think the issue technically is that a) loaders now run in worker threads (something to do with not corrupting the global environment), and b) If you want a worker thread itself to us a loader, basically you can't feed the option through. I think this is the active bug on nodejs: nodejs/node#47747

The first problem is that the ts node loader has changed and doesn't work with ava. It works if I use a thing called tsimp, but it builds all the test files to disk. Not essential I suppose but it annoys me. Also the docs say it does everything virtually, so that's kinda confusing.

I suppose one solution here is to just wait this out and still on node 18 for a while. It doesn't affect the runtime, only the build-time.

The second problem is some changes about importing folders, which we can't do any more. I suppose we'll just have re-write the affected code, although that could be a big job

@josephjclark
Copy link
Collaborator Author

Running ava with workerThreads off (forcing it to use child processes) works fine, at the cost of using more system resources (and so being slower in CI).

What I'm confused at with this is that I don't get import errors for importing folders and stuff, which I do get when using other loaders. Is that just a config thing?

@josephjclark
Copy link
Collaborator Author

Aside from the ava issue, I think we have two other problems holding back the update

  1. bumping node is likely to cause a bump in pnpm version. We should really setup corepack and lock the repo to pnpm 8. Once we're on the latest node, we can do a pnpm update in isolation (it really shouldn't affect anything but lets divide and conquer on the risk)

  2. I think the build in describe-package is broken for the same reason - we're using node loaders with esbuild to build that package, and those loaders are also broken. Hopefully we can apply the same solution for ava AND esbuild 🤞

@josephjclark
Copy link
Collaborator Author

See also #542

@doc-han doc-han self-assigned this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

3 participants