Add type-strip mode: run Harper directly from .ts sources#562
Draft
kriszyp wants to merge 69 commits into
Draft
Conversation
Introduces utility/lifecycle.ts with onStartup(cb) / runStartup() so side-effectful module-load work (server-singleton wiring, listener registration, config-derived constants) can be deferred to a controlled startup phase that runs after env.initSync() and before request handling. This is the lever that makes ESM-cycle TDZs avoidable without restructuring the import graph. Adds RUNTIME_SRC_ROOT and RUNTIME_FILE_EXT to packageUtils.js. Detects whether the running modules are .ts sources (type-strip) or .js dist files by inspecting packageUtils.js's own __dirname, so callers like manageThreads.startWorker() can spawn workers with the correct file path under both modes from a single string identifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames the 57 .js source files that participate in Harper's runtime
module graph to .ts and rewrites them in ESM form. The driving constraint:
under Node's type-strip mode, .ts files with import/export syntax are
loaded as ESM, and any CJS .js file that require()s a .ts ESM file in the
entry's evaluation chain throws ERR_REQUIRE_CYCLE_MODULE.
Mechanical edits applied across all converted files:
- const X = require('mod') → import X from 'mod'
- const { A, B } = require('mod') → import { A, B } from 'mod'
- const X = require('mod').default → import X from 'mod'
- module.exports = { a, b, ... } → export { a, b, ... }
- module.exports = X → export default X
- 'use strict' pragma removed (ESM default)
- Relative imports given explicit .ts/.js extensions
These files keep their existing internal structure and behavior; the
conversion is syntactic, not semantic. Behavioral changes (deferred
side-effects, late-binding, etc.) live in the follow-up commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the runtime support needed to run Harper directly from .ts sources
under Node's type-strip mode (node bin/harper.ts) while keeping the
existing CJS dist build (node dist/bin/harper.js) working unchanged.
Entry point (bin/harper.ts):
- Switch cases use await import('./mod.ts') instead of require('./mod')
- Splits boot into initEnv() (always cheap) and runServerStartup() (only
for start/run paths); version/help skip the heavy init
- Replaces require.main === module with a dual-mode isEntry check
Worker thread bootstrap (server/threads/threadServer.ts):
- Workers run env.initSync() → runStartup() → startServers() in an async
IIFE so they see a populated env and complete onStartup hooks before
serving requests
manageThreads.startWorker():
- Accepts an extensionless module identifier and appends RUNTIME_FILE_EXT
- Resolves against RUNTIME_SRC_ROOT (source dir under type-strip,
dist/ under CJS dist)
- listenersByType / messageListeners lazy-initialized inside the accessor
so callers don't trip TDZ when manageThreads is loaded mid-cycle
Late-binding for class-extends-Resource modules:
- ErrorResource, CertificateVerificationSource,
CertificateRevocationListSource, Login: the class is constructed
inside onStartup() and exposed via `export let X` so consumers see a
live binding after startup. These modules sit inside Resource.ts's own
static-graph SCC, so a class-extends declaration at module-top TDZ'd.
Module-load side-effect deferrals (one-off onStartup wrappers):
- server.X = … assignments in security/user.ts, security/auth.ts,
resources/analytics/write.ts, server/http.ts,
server/serverHelpers/serverUtilities.ts, server/threads/threadServer.ts
- Object.defineProperty(server, …) in server/nodeName.ts and
server/threads/manageThreads.ts
- onMessage* listener registrations in server/threads/itc.ts,
security/keys.ts, resources/analytics/write.ts,
utility/processManagement/processManagement.ts, bin/restart.ts
- Config-derived constants in security/auth.ts and
server/serverHelpers/contentTypes.ts converted to lazy getters
- server.contentTypes attachment in contentTypes.ts deferred
- whenComponentsLoaded-awaiting IIFE in
server/DurableSubscriptionsSession.ts deferred
Environment manager (utility/environment/environmentManager.ts):
- env.get() and getHdbBasePath() made defensive against being called
before this module's body has run (catches TDZ ReferenceError from
configUtils.getConfigValue and returns undefined). Belt-and-suspenders
for the rare timing window where a module-load access reaches in
before initSync completes.
- Removed 22 eager module-top env.initSync() calls; initialization is
now centralized in bin/harper.ts (initEnv) and threadServer.ts
- Top-level let/const → var so subsequent functions don't TDZ on
module-internal state if invoked through the cycle
harper_logger.ts:
- RootConfigWatcher import moved from static (which forced a
configUtils → logger cycle) to dynamic, inside the function that
actually constructs the watcher
- logRotator require replaced with dynamic import inside its setTimeout
- module.exports = {…} wrapped in `if (typeof module !== 'undefined')`
so the CJS-shaped legacy export survives tsc emit but is skipped
under ESM, where the parallel export default already covers the
default-import consumers
Compatibility fixes:
- Type-only imports tagged via tsc --verbatimModuleSyntax sweep
- CJS-only packages (lodash, fs-extra, micromatch, papaparse, etc.)
switched to default-import-and-destructure
- JSON imports tagged with `with { type: 'json' }`
- Extensionless relative imports given explicit .ts/.js extensions
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`(mod.default || mod)()` only worked when `await import()` returned an
ESM-shaped namespace (mod.default = function). In tsc-compiled CJS dist,
`await import('./install.js')` from a CJS file wraps the CJS exports as
mod.default, so the actual function is at mod.default.default. CI's
`Setup Harper` step (which runs the dist build) hit this with
`TypeError: (mod.default || mod) is not a function`.
Adds a small getDefaultExport() helper that walks both shapes and applies
it to the INSTALL, STOP, and STATUS cases in bin/harper.ts.
Verified locally:
- node bin/harper.ts install → reaches installer
- node dist/bin/harper.js install → reaches installer
- node bin/harper.ts version → 5.1.0
- node bin/harper.ts → "Harper successfully started."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
2 blockers still open — the node-forge default-export fix (d2cf35e) is correct and introduces no new issues. Blocker 1 —
Fix: Blocker 2 —
Fix: |
Issues caught by the claude/review bot and the unit-test job:
1. bin/run.ts:235 (`launch()`) — `require('../utility/processManagement/processManagement.ts')` would throw `ReferenceError` under typestrip ESM. Converted to `await import(...)` (mirrors `filterArgsAgainstRuntimeConfig` a few lines above).
2. server/threads/threadServer.ts (`onSocket`) — `require('../../components/componentLoader.ts').getComponentName` had the same problem on the secure-port code path. Converted onSocket to async and used `await import(...)`. Inspector calls in the same file (debug-mode paths) now use a single static `import * as inspector from 'node:inspector'` instead of per-call requires, so they work in both modes.
3. utility/functions/sql/alaSQLExtension.ts — `import mathjs from 'mathjs'` got `undefined` for `mathjs.mad` etc. since mathjs is ESM-only and has no default export. Switched to `import * as mathjs` and replaced the trailing `module.exports = {...}` with `export default {...}` so the file is consistently ESM. This was the cause of `TypeError: Cannot read properties of undefined (reading 'mad')` that surfaced in unit-test runs.
Verified both modes still reach "Harper successfully started."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
force-pushed
the
claude/typestrip-mode
branch
from
23a2e4d to
ded6937
Compare
Addresses the second round of claude/review findings plus the
ErrorResource regression Gemini flagged that showed up in unit tests:
- server/threads/socketRouter.ts:31 — `await require('./threadServer.ts').startServers()` was half-converted (extension updated but `require` left); switched to `await (await import(...)).startServers()`. Triggered when threadCount === 0 in typestrip mode.
- bin/upgrade.ts:36 — `pm2Utils = require('../utility/processManagement/processManagement.ts')` parallel to the `pmUtils` fix already done in bin/run.ts; switched to `await import(...)`. Was breaking `harper upgrade` in typestrip mode.
- utility/signalling.ts — `serverItcHandlers = serverItcHandlers || require('../server/itc/serverHandlers.ts')` in signalSchemaChange/signalUserChange; both functions made async and switched to `await import(...)`.
- server/fastifyRoutes/plugins/hdbCore.js — renamed to .ts and converted to ESM. As a CJS file it required serverHandlers.ts (now ESM), throwing ERR_REQUIRE_CYCLE_MODULE under typestrip when fastify routes load. The single importer (server/fastifyRoutes.ts) updated to point at .ts.
- resources/ErrorResource.ts — rewrote to lazy-construct the class on first
use via a Proxy. The earlier `onStartup`-based late binding worked in
production (where runStartup runs before componentLoader can construct
ErrorResource) but broke unit tests that exercise componentLoader without
going through the lifecycle, producing the `ErrorResource is not a
constructor` failure. The Proxy resolves on `new` or property access, so
the class is always available regardless of lifecycle state, and still
dodges the original module-load TDZ on Resource.
Verified locally:
- node bin/harper.ts version → 5.1.0
- node bin/harper.ts → Harper successfully started.
- node dist/bin/harper.js → Harper successfully started.
- npm run lint:required → 0 errors, 0 warnings
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Harper's unit tests use `rewire` to override module-internal bindings via `__set__` / `__get__`. Rewire works by `eval`-injecting code into the module's closure, so it needs a variable with the exact name the test references. `tsc`'s CJS emit renames `import X from 'mod'` to a `_X_ts_1` alias internally and rewrites all uses to `_X_ts_1.default`. After commit 1f4522a converted the underlying source files from CJS `.js` to ESM `.ts`, the dist `_ts_1` aliases broke ~95 rewire-using unit tests that expected to find a variable named `X` in scope. This commit adds a `const X = _X` alias right after each affected default/namespace/named import in the 21 source files that rewire-using tests target. tsc emits this as a real module-scope `const X = _X_ts_1.default;` (or equivalent), restoring the variable name rewire looks for. ESM (typestrip) sees the same `const X = _X;` trivially. Files touched (each only adds the alias for the specific names the test rewires; the rest of the imports are left alone): bin/upgrade.ts components/operations.ts config/configUtils.ts dataLayer/bulkLoad.ts dataLayer/delete.ts dataLayer/export.ts dataLayer/insert.ts dataLayer/readAuditLog.ts dataLayer/schema.ts dataLayer/schemaDescribe.ts dataLayer/update.ts dataLayer/harperBridge/lmdbBridge/lmdbMethods/{lmdbCreateSchema,lmdbCreateTable,lmdbDropTable,lmdbUpsertRecords}.ts security/fastifyAuth.ts server/serverHelpers/serverHandlers.ts sqlTranslator/sql_statement_bucket.ts validation/role_validation.ts validation/schemaMetadataValidator.ts validation/validationWrapper.ts Sampled local results after applying: - upgrade.test.js: 4 passing (was: fail in before hook) - validationWrapper.test.js: 8 passing (was: ReferenceError validate not defined) - insert.test.js: 16 passing - delete.test.js: 12 passing - sql_statement_bucket.test.js: 16 passing - configUtils.test.js: 40 passing, 1 unrelated failure Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
force-pushed
the
claude/typestrip-mode
branch
from
2b250f2 to
31b0ef6
Compare
Round of follow-up fixes after the typestrip conversion exposed several
test failures in the dist (CJS-emit) build:
- utility/functions/geo.ts: turfArea, turfLength, turfCircle,
turfDistance, turfBooleanContains, turfBooleanEqual,
turfBooleanDisjoint were called as `.default(...)` (CJS interop
pattern) but the new ESM `import X from '@turf/X'` already resolves to
the default. Removed the `.default` accessors. Also switched
`@turf/helpers` to `import * as` since it only exposes named exports.
(geo unit tests now pass: 79/79)
- server/serverHelpers/contentTypes.ts: my earlier rsync was off a
pre-x-ndjson revision of main, so the rsync overwrote main's
application/x-ndjson handler. Restored it.
(contentTypes test: 16/16 passing)
- utility/signalling.ts: my prior fix made signalSchemaChange /
signalUserChange async (to `await import('serverHandlers.ts')`),
but the existing tests and callers assumed sync. Restored sync
signatures and moved the import to an `onStartup` preload + safe
optional-chain fallback for environments where startup never runs.
(signalling tests: 4/4 passing)
- utility/install/checkJWTTokensExist.ts: original CJS used
`module.exports = fn` so `require('./checkJWTTokensExist')(...)` was
callable directly. My `export default fn` converted to
`exports.default = fn`, breaking that calling convention. Added a
conditional `module.exports = fn` shim that runs only when `module`
is defined (tsc CJS emit) and is skipped under typestrip ESM.
(checkJWTTokensExist tests: 2/2 passing)
- unitTests/testUtils.js: `preTestPrep()` now calls
`lifecycle.runStartup()` after `initTestEnvironment()`. Many tests
stub server-singleton methods (e.g. `sinon.stub(server, 'getUser')`)
that are now wired during the startup phase rather than at module
load. Calling runStartup() in preTestPrep gives tests the same
fully-wired state production sees. (`runStartup` is idempotent.)
(auth.test.js: 10/10 passing)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…taller) The rewire-target extractor missed two patterns in the first pass: - Variable-based rewire calls like `rewire(MODULE_PATH)` where MODULE_PATH is a local string constant - The harper_logger PropertiesReader import (via the requireUncached helper) Applies the same const-alias-after-import pattern to: - utility/logging/harper_logger.ts: PropertiesReader - utility/install/installer.ts: PropertiesReader, installValidator, mountHdb, checkJwtTokens Local check: harper_logger.test.js now 24/24 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The runtime-env-vars test used `__set__('require', stub)` to intercept the
inline `require('./harperConfigEnvVars.ts')` call in
`applyRuntimeEnvVarConfig`. The ESM conversion hoisted that require to a
top-level import, leaving the test stub unable to substitute anything and
the function ran against the real fs (which then errored on `/test/root`).
Adds the const-alias pattern for `applyRuntimeEnvConfig` in configUtils.ts
so rewire can patch the binding directly, and updates the test to
`__set__('applyRuntimeEnvConfig', stub)` instead of the old require-trap.
13/13 in that test file passing now.
Two issues uncovered while investigating integration-test regressions:
- dataLayer/{insert,delete,readAuditLog}.ts imported `harperBridge` via
`import _harperBridge ...; const harperBridge = _harperBridge;`. Under
the existing intentional cycle (insert.ts ↔ harperBridge.ts via the
bridge's transitive deps), `_harperBridge` is in TDZ at the moment
the alias evaluates, so `harperBridge` was bound to `undefined`
forever and the daemon crashed at startup with
`Cannot access '_harperBridge' before initialization`. Replaced the
capture with a Proxy that defers `.X` reads to call time — the cycle
is fully resolved by the time any usage fires, and rewire tests can
still patch the binding name.
- server/jobs/jobRunner.ts used `join(__dirname, './jobProcess.js')` to
resolve the job-worker entry path. `__dirname` is undefined under
type-strip ESM, so the call threw silently when a job was launched
(which silently meant CSV/bulk-load jobs never spawned a worker).
Switched to an extensionless identifier `'server/jobs/jobProcess'`
so `manageThreads.startWorker()` resolves it against `RUNTIME_SRC_ROOT`
with the correct extension per execution mode.
Note: integration-test CSV bulk-load is still broken locally — the job
worker startup hangs without reaching the actual data path. The above
fixes are necessary preconditions but not sufficient; ongoing
investigation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mports
claude/review flagged that the LMDB bridge files still had top-level
\`require('./path').default || require('./path')\` shims that throw
\`ReferenceError: require is not defined\` under typestrip ESM. Same in
the launchHarperDB entry script and mount_hdb's createTables.
Converted via regex sweep — pattern was uniform across these files:
\`const X = require('./path').default || require('./path');\`
becomes \`import X from './path';\`. Where the require pattern was
spread across multiple lines (triple-require defensive shims in
lmdbCreateTransactionsAuditEnvironment), collapsed to a single import.
launchServiceScripts/launchHarperDB.ts: top-level \`require(...)\` call
replaced with a dynamic \`import(...).then(...)\`.
utility/mount_hdb.ts: inline require inside \`createTables\` hoisted to
a top-of-file import (CreateTableObject).
Verified \`node bin/harper.ts version\` still works. \`npm run lint:required\`
clean, \`npm run format:write\` no changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The destructured SearchByConditionsObject/SearchCondition were only referenced from JSDoc @param annotations — under typestrip mode the top-level require() throws ReferenceError. The TS file is already typed; the JSDoc-only imports add no value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
requested review from
Ethan-Arrowood and
cb1kenobi
and removed request for
a team
Member
|
Trying to test this with Node 24 and running There's a couple places where we import .json files and I believe we need the |
Static JSON imports (`import x from './file.json'`) require
`with { type: 'json' }` in Node 24 ESM/typestrip mode, but TypeScript
with module:NodeNext + CJS output raises TS2856 if the attribute is
present.
Replace all six static systemSchema JSON imports with synchronous
`JSON.parse(readFileSync(...))` using `PACKAGE_ROOT` from
`utility/packageUtils.js` for path resolution. This pattern:
- Works in typestrip mode (ESM, Node 24)
- Works in CJS dist (Node 20+)
- Emits no TS2856 errors
- Is consistent with how other JSON files are loaded in the codebase
Files changed: utility/globalSchema.ts, utility/mount_hdb.ts,
security/user.ts, dataLayer/.../lmdbSearch.ts,
dataLayer/.../lmdbDropAttribute.ts, upgrade/directives/5-2-0.ts
Closes reviewer feedback from cb1kenobi.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed in e44864a. Replaced all six static The — Claude |
The unit test was injecting `systemSchema_json_1` (the CJS name tsc
emits for `import systemSchema from './file.json'`). The previous commit
replaced the static JSON import with `const systemSchema = JSON.parse(…)`,
so the rewire target is now `systemSchema` directly. Update the test to
match and drop the `{ default: … }` wrapper that the import helper added.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Single conflict in components/operations.ts: kept ESM imports from HEAD and picked up main's new `awaitDeploymentRow` export from deploymentRecorder.ts + `onPeerResult` arg to replicateOperation. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Conflicts in 5 files (serverHandlers.ts, manageThreads.ts,
threadServer.ts, operationsServer.ts, fastifyRoutes.ts) — all the same
pattern: main introduced `workerProcessGuard.ts` with a `realExit()`
wrapper around process.exit() for cleaner worker shutdown. Kept our ESM
imports, added `import { realExit }` and replaced `process.exit(0)` with
`realExit(0)` in the SHUTDOWN handler.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Conflict in server/itc/serverHandlers.ts: main added named exports for userHandler and schemaHandler (for MCP listChanged subscribers). Kept our ESM `export default` and converted the new exports to ESM style. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
| startServers(); | ||
| })().catch((err) => { | ||
| harperLogger.fatal('Worker failed to start', err); | ||
| process.exit(1); |
Contributor
There was a problem hiding this comment.
Blocker (new) — fatal worker startup error is now silently swallowed
workerProcessGuard.ts is imported at line 18 of this file, so by the time this catch block runs in a worker thread the guard has already replaced process.exit with a logging stub. process.exit(1) here is intercepted — the worker logs the fatal error but continues running in a broken state instead of terminating.
Every other fatal exit in this commit (fastifyRoutes.ts, jobProcess.ts, operationsServer.ts) was updated to realExit() — this one was missed.
Suggested change
| process.exit(1); | |
| realExit(1); |
The review caught that the fatal worker startup catch block still called process.exit(1) instead of realExit(1). Since workerProcessGuard.ts is imported at the top of threadServer.ts, process.exit is already replaced by the guard's logging stub by the time this catch fires — the worker would log the error but continue running in a broken state. Every other fatal exit in this file was already updated in the merge commit; this one was missed. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed comment 3329139859 in c57a656 — changed |
Member
|
First try I get: |
Static and dynamic imports of TypeScript source files were still using
.js extensions in several files, causing ERR_MODULE_NOT_FOUND when
running under Node 24 type-strip mode (node bin/harper.ts).
Files fixed:
- upgrade/directives/5-2-0.ts: initializePaths.js → .ts
- components/deployLifecycle.ts: manageThreads.js → .ts
- components/mcp/index.ts: configUtils.js → .ts
- server/operationsServer.ts: configUtils.js, itc.js, manageThreads.js → .ts
- security/certificateVerification/certificateVerificationSource.ts:
crlVerification.js, ocspVerification.js → .ts (dynamic imports)
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The previous commit ran `git add` without GIT_WORK_TREE set, causing git to resolve the path relative to the git-dir root instead of the worktree root. Only certificateVerificationSource.ts was tracked correctly (the sed-i broke its hard-link, giving it a new inode that git could resolve). The other 8 files in the directory appeared deleted in the index because git couldn't open the directory. Re-add all files with GIT_WORK_TREE explicitly set to restore the directory to the committed state. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed in 927c9d3 + d724e76. Converted 6 remaining — Claude |
The previous .js→.ts commit used sed -i which breaks hard links by
creating a new inode. When git add ran without GIT_WORK_TREE set, git
resolved the wrong worktree root and staged an older version of
operationsServer.ts (from the main checkout's hard-linked copy),
accidentally reverting:
- env.initSync() try/catch wrapper
- `type` keyword on fastify imports
- requestTimePlugin.ts and serverHandlers.ts extensions
- operationsServer default parameter `= {}`
The correct on-disk versions of all 4 affected files are now staged
using GIT_WORK_TREE=$(pwd) to ensure the worktree root resolves
correctly in this submodule worktree setup.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
|
Getting closer: |
In typestrip mode (node bin/harper.ts), `require` is undefined in ESM
modules. common_utils.ts had 6 lazy `require('../resources/databases')`
calls inside function bodies to break circular-import cycles.
Replace with a top-level ESM import of getDatabases and a thin wrapper
function. The circular dependency is safe because these functions are
only called at runtime, never during module initialization.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed in 7d8b12d. The 6 — Claude |
common_utils.ts → databases.ts → itc.ts → common_utils.ts created a circular dependency that caused the 'restart' ITC listener to not register on the main thread, breaking restart_service jobs (workers couldn't restart HTTP workers after component hot-reload). Fix: inline isEmpty() directly in itc.ts (it was the only function itc.ts used from common_utils) so itc.ts no longer imports common_utils. This breaks the cycle and allows common_utils.ts to safely add a top-level import of getDatabases from databases.ts. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
|
…level import
Previous approach (direct top-level import) created circular deps with
multiple modules: databases.ts → Table.ts / auditStore.ts / DatabaseTransaction.ts
→ common_utils.ts, all of which import back into common_utils.ts.
New approach:
- utility/databasesRef.ts: tiny dep-free registry with setDatabasesGetter()
and getDatabases()
- resources/databases.ts: calls setDatabasesGetter(() => getDatabases()) at
module top-level, after its own getDatabases export is defined
- utility/common_utils.ts: imports getDatabases from databasesRef.ts (no cycle)
- server/threads/itc.ts: inlines isEmpty to remove its import of common_utils
(that import would have created a cycle via databases→itc→common_utils)
This eliminates all `require()` calls from common_utils.ts while keeping the
lazy-loading semantics needed to avoid circular dependency issues.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
In ESM (node bin/harper.ts), `import _validate from 'validate.js'`
returns the CJS module wrapped as `{ default: validateFn }`. The old
`import * as validate from 'validate.js'` therefore left `validate`
pointing at the namespace object, making `validate.cleanAttributes`
undefined.
Use `_validate.default ?? _validate` to handle both ESM (where .default
is the library) and CJS dist (where _validate already IS the library).
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed in 3ae1a25. In ESM typestrip mode, — Claude |
Member
|
This PR is still broken. Please move this PR back to draft. It's not ready for review. |
Same pattern as validate.js: node-forge is CJS and gets wrapped as
{ default: forge } when imported in ESM. `import * as forge` then
returns the namespace instead of the library, making forge.pki
undefined and causing `publicKeyFromPem` to fail during install.
Use `_forge.default ?? _forge` to unwrap the default export in ESM
while staying backward-compatible with the CJS dist.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
Fixed in d2cf35e. Same CJS default-export wrapping issue as validate.js: Also moving the PR back to draft as requested while continuing to track and fix remaining issues. — Claude |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lets Harper run from the same source tree in two modes:
node bin/harper.ts— Node 24's built-in type-strip (ESM, runs .ts directly)node dist/bin/harper.js— existing CJS dist build (unchanged shipped artifact)Both reach
Harper successfully started.for the daemon, and produce identical output forversion/help.Why
Running .ts directly removes the build step from the inner dev loop and gives us a path to drop the tsc-CJS emit eventually. The challenge is that Harper's runtime module graph is full of CJS↔ESM cycles and module-load side effects (config reads, server-singleton wiring, listener registrations, class-extends-Resource) that work under tsc's synchronous CJS but TDZ under Node's ESM evaluation.
Approach (three commits)
utility/lifecycle.ts(onStartup(cb)/runStartup()) plusRUNTIME_SRC_ROOT/RUNTIME_FILE_EXTexports fromutility/packageUtils.jsso callers can distinguish source vs. dist at runtime without__dirname/import.metashenanigans..jssource modules to ESM.ts— 57 mechanical renames (require→import,module.exports→export, etc.). This is the bulk of the diff.bin/harper.tsreshaping, worker bootstrap,onStartupwrappers, late-binding forResourcesubclasses, defensiveenv.get, removal of eager module-topenv.initSync()calls.Where to focus review attention
Higher-confidence:
utility/lifecycle.ts) — small, contained.RUNTIME_SRC_ROOT/RUNTIME_FILE_EXTadditions topackageUtils.js— purely additive..js→.tsconversion in commit 2.Lower-confidence — please scrutinize:
Late-bound Resource subclasses —
resources/ErrorResource.ts,security/certificateVerification/{certificateVerificationSource,crlVerification}.ts, and theLoginclass inresources/login.tsnow create their class insideonStartup()and export viaexport let X. In tsc's CJS emit, this becomesexports.X = void 0thenexports.X = class …inside the hook. Caveat for future contributors: module-top destructuring of these exports (const { ErrorResource } = require('./ErrorResource')) would captureundefined. All current consumers access viamod.Xat use-site (or destructure inside a function body), which sees the live property after startup — but this is a sharp edge worth knowing about.Defensive
env.get/getHdbBasePathinutility/environment/environmentManager.ts— both now tolerate being called before this module's body has fully run (catchReferenceErrorfromconfigUtils.getConfigValueTDZ; optional-chaininstallProps?.). Belt-and-suspenders for cycle timing; could mask a real "called before init" bug. Now thatinitEnv()always runs before any subcommand module loads, this might be removable in a follow-up.isEntrycheck inbin/harper.ts:Works because nothing currently imports
bin/harper.tsas a module. If anyone ever does (test runner, wrapper script), the CLI side-effect would run unexpectedly. Hardening this would need eitherimport.meta.url(which tsc errors on under CJS emit) or a separate ESM/CJS entry shim.Worker
RUNTIME_FILE_EXTswitch inmanageThreads.startWorker()— the function now accepts extensionless module identifiers ('server/threads/threadServer') and resolves them againstRUNTIME_SRC_ROOTwith.tsor.jsper mode. Verify this matches how external consumers callstartWorker.Removed eager module-top
env.initSync()calls (22 sites). Initialization is centralized inbin/harper.ts(initEnv) andthreadServer.tsfor workers. Tests that import these modules directly will need to callinitSync()themselves.Test implications
server.Xbeing wired,env.get(K)returning a real value, or worker-thread message listeners being registered must callrunStartup()(exported fromutility/lifecycle.ts) before exercising those paths.resetStartupForTests()is provided for inter-test isolation.Verification
Process notes
.js→.tsrename in commit 2; commits 1 and 3 are where the actual design decisions live and warrant the closest read.geminibefore pushing; its flags addressed inline above.🤖 Generated by Claude Code (Opus 4.7, 1M context).