-
Notifications
You must be signed in to change notification settings - Fork 15
Update to Node 24 #1331
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
base: release/next
Are you sure you want to change the base?
Update to Node 24 #1331
Changes from all commits
572c82a
92ed2e7
cc03d78
4016e4b
b72e8a8
ea4818d
88cc093
bf55c75
bef5c71
f008a08
80ac6ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@openfn/runtime': minor | ||
| --- | ||
|
|
||
| - Enable full compatibility with node 24 | ||
| - When loading modules, prefer ESM targets over CJS targets |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| nodejs 22.12.0 | ||
| nodejs 24.14.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| nodejs 24.14.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,4 @@ | ||
| import { createMockLogger } from '@openfn/logger'; | ||
| import createLightningServer, { | ||
| DEFAULT_PROJECT_ID, | ||
| } from '@openfn/lightning-mock'; | ||
| import test from 'ava'; | ||
| import mock from 'mock-fs'; | ||
| import { execSync } from 'node:child_process'; | ||
|
|
@@ -22,16 +19,6 @@ const TIMEOUT = 1000 * 30; | |
|
|
||
| const logger = createMockLogger('', { level: 'debug' }); | ||
|
|
||
| const port = 8967; | ||
|
|
||
| let server; | ||
|
|
||
| const endpoint = `http://localhost:${port}`; | ||
|
|
||
| test.before(async () => { | ||
| server = await createLightningServer({ port }); | ||
| }); | ||
|
|
||
| test.afterEach(() => { | ||
| mock.restore(); | ||
| logger._reset(); | ||
|
|
@@ -75,15 +62,16 @@ async function run(command: string, job: string, options: RunOptions = {}) { | |
| // This is needed to ensure that pnpm dependencies can be dynamically loaded | ||
| // (for recast in particular) | ||
| const pnpm = path.resolve('../../node_modules/.pnpm'); | ||
| const pkgPath = path.resolve('./package.json'); | ||
|
|
||
| const pkgPath = path.resolve('./package.json'); | ||
| const recastPath = `${pnpm}/recast@0.21.5`; | ||
| // Mock the file system in-memory | ||
| if (!options.disableMock) { | ||
| mock({ | ||
| [expressionPath]: job, | ||
| [statePath]: state, | ||
| [outputPath]: '{}', | ||
| [pnpm]: mock.load(pnpm, {}), | ||
| [recastPath]: mock.load(recastPath, {}), | ||
| // enable us to load test modules through the mock | ||
| '/modules/': mock.load(path.resolve('test/__modules__/'), {}), | ||
| '/repo/': mock.load(path.resolve('test/__repo__/'), {}), | ||
|
|
@@ -838,22 +826,3 @@ test.serial( | |
| ); | ||
| } | ||
| ); | ||
|
|
||
| test.serial('pull: should pull a simple project', async (t) => { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this because it's failing, but we have better coverage of this in integration tests now |
||
| t.timeout(TIMEOUT); | ||
| mock({ | ||
| './state.json': '', | ||
| './project.yaml': '', | ||
| }); | ||
| process.env.OPENFN_ENDPOINT = endpoint; | ||
|
|
||
| const opts = cmd.parse(`pull ${DEFAULT_PROJECT_ID}`) as Opts; | ||
| await commandParser(opts, logger); | ||
|
|
||
| const last = logger._parse(logger._history.at(-1)); | ||
| t.is(last.message, 'Project pulled successfully'); | ||
| const errors = logger._find('error', /./); | ||
| t.falsy(errors); | ||
|
|
||
| delete process.env.OPENFN_ENDPOINT; | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,27 +231,25 @@ export const getModuleEntryPoint = async ( | |
| 'utf8' | ||
| ); | ||
| const pkg = JSON.parse(pkgRaw); | ||
| let main = 'index.js'; | ||
|
|
||
| // TODO Turns out that importing the ESM format actually blows up | ||
| // (at least when we try to import lodash) | ||
| // if (pkg.exports) { | ||
| // if (typeof pkg.exports === 'string') { | ||
| // main = pkg.exports; | ||
| // } else { | ||
| // const defaultExport = pkg.exports['.']; // TODO what if this doesn't exist... | ||
| // if (typeof defaultExport == 'string') { | ||
| // main = defaultExport; | ||
| // } else { | ||
| // main = defaultExport.import; | ||
| // } | ||
| // } | ||
| // } else | ||
| // Safer for now to just use the CJS import | ||
| if (pkg.main) { | ||
| main = pkg.main; | ||
| // Find the best ESM entrypoint | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Scary change here It looks like the problems I'm (luckily!) catching in tests is that node24 has changed the module loader a bit and support for CJS feels flaky. That's OK, I'd much rather be using ESM anyway. So this change prefers the ESM import to the CJS one. I just worry about whether things are going to break?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, some things break. Some older packages fail to import through the ESM loader (basically if they import paths which don't have a .js extension) |
||
| // https://nodejs.org/api/packages.html#package-entry-points | ||
| let esm; | ||
| if (typeof pkg.exports === 'string') { | ||
| esm = pkg.exports; | ||
| } else { | ||
| const exportsField = pkg.exports?.['.']; | ||
| esm = | ||
| typeof exportsField === 'string' ? exportsField : exportsField?.import; | ||
| } | ||
|
|
||
| // main might point to esm or cjs, but in our adaptors it points to CJS | ||
| const cjsProbably = pkg.main; | ||
|
|
||
| const main = esm ?? cjsProbably ?? 'index.js'; | ||
|
|
||
| const p = path.resolve(moduleRoot, main); | ||
|
|
||
| return { path: p, version: pkg.version }; | ||
| } | ||
| return null; | ||
|
|
||
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.
bonus: this will make tests run quicker. Should I do this in more places?