Repository navigation
fix(configorama): load/process-mode syntax errors and named exports in file refs - #87
Merged
Merged
Conversation
…process mode
With moduleCacheMode 'load' or 'process', loadExecutable compiles each
.js/.mjs/.ts file itself with jiti's transform() and runs the result in
vm. On a syntax error transform() does not throw: it returns code that
sets `exports.__JITI_ERROR__ = { filename, line, column, code, message }`.
jiti's own require checks for that marker and throws; loadExecutable did
not, so the marker object became the module's exports. A
`${file(./broken.mjs)}` ref resolved to `{ __JITI_ERROR__: ... }` and a
broken root config loaded as that object.
loadExecutable now checks for the marker after running the module and
throws `<code>: <message> (<file>:<line>:<column>)`, the same message
shape jiti's require uses. The existing catch then drops the module
from the graph, so the next load re-reads the file. The resolver and
parseFileContents wrap the error as they do for legacy mode
("Error processing ESM file: ...", "Failed to execute TypeScript file
<path>: ..."). Legacy mode is unchanged: it goes through jiti's require
(.mjs), tsx (.ts) or Node's require (.js), which all throw already.
Nothing in the repo reads __JITI_ERROR__; it is jiti's internal marker.
Test: tests/reliability/moduleLifecycle.test.js covers .js, .mjs and .ts
in load and process modes, as a ${file()} ref and as the root config,
through both configorama() and configorama.sync().
…rocess mode
In moduleCacheMode 'load' or 'process', loadExecutable returns a module
that has a default export as a Proxy whose target is the default export;
its get trap serves named exports from the module's exports. So
`proxy.a` works, but `hasOwnProperty(proxy, 'a')` checks the default
export and is false for a named export.
selectModuleExport used that hasOwnProperty check to decide whether
`:a` names an export. For a non-function named export it fell through
to the main-export branch, read `a` from the default export, got
undefined, and `${file(./x.mjs):a}` / `${file(./x.ts):a}` failed with
"Invalid variable syntax". A named function export happened to work
through the "function on the main export" branch. The .js branch never
checks ownership (it reads `jsFile[name]` through the proxy), so the
same module as .js returned the value.
loadExecutable now records each interop proxy's exports object in a
WeakMap and exposes `moduleExports(value)`, which returns those exports
(or the value itself when it is not a proxy). selectModuleExport checks
ownership on that, so a named export wins over the default export as it
does in legacy mode, where jiti's own interop proxy targets the module
namespace. Values are still read through the proxy; nothing else about
the proxy changes.
Test: tests/reliability/moduleLifecycle.test.js resolves `:a` and `:b.c`
from .js, .mjs and .ts modules (the .mjs/.ts ones with a default export)
in legacy, load and process modes, async and sync, and expects the same
values for all three.
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
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.
Fixes two of the three bugs found during the 2026-10-24 simplify loop. The third (
varRegex) is not changed; see the last section for why.Both fixed bugs only show up with
moduleCacheMode: 'load'or'process'. In the default'legacy'mode,.mjsgoes through jiti's require,.tsthrough tsx and.jsthrough Node's require, and all three already behave correctly.1. Syntax errors in
.js/.mjs/.tsresolved to{ __JITI_ERROR__: ... }(2e0966f)Bug: In load/process mode,
loadExecutablecompiles modules itself with jiti'stransform()and runs them invm. On a syntax error,transform()doesn't throw. It returns code that setsexports.__JITI_ERROR__ = {...}. jiti's own require checks for that marker andloadExecutabledidn't, so the marker became the module's exports.${file(./broken.mjs)}resolved to{ __JITI_ERROR__: {...} }, and a broken root config loaded as that object. This also affected.js, not just.mjs/.ts.Fix: After running the module,
loadExecutablechecks for the marker and throws<code>: <message> (<file>:<line>:<column>), the same message shape jiti's require uses. The existing catch drops the module from the graph, and callers wrap the error as they do in legacy mode (e.g.Error processing ESM file: ParseError: Unexpected token (/path/broken.mjs:2:0)). Nothing in the repo reads__JITI_ERROR__.2.
${file(./x.mjs):a}/${file(./x.ts):a}failed for a non-function named export (f99704e)Bug: In load/process mode, a module with a default export comes back as a Proxy whose target is the default export, and a
gettrap serves the named exports.selectModuleExportusedhasOwnProperty(fileModule, name)to decide whether:anames an export. On the proxy that check looks at the default export, so it was false. The lookup then readafrom the default export and gotundefined, which surfaced as "Invalid variable syntax". The.jsbranch readsjsFile[name]through the proxy and never checks ownership, so the same module as.jsworked.Fix:
loadExecutablerecords each interop proxy's exports object in a WeakMap and exposesmoduleExports(value).selectModuleExportchecks ownership on those exports, so a named export takes precedence over the default export, as it does in legacy mode. Values are still read through the proxy.New tests
Both are in
tests/reliability/moduleLifecycle.test.js, next to the existing load/process-mode tests. Each one failed before its fix..js,.mjsand.tsfiles with a syntax error, in load and process modes, used both as a${file()}ref and as the root config, through bothconfigorama()andconfigorama.sync(). Each case must throw an error that names the file.:aand:b.cfrom.js,.mjsand.tsmodules (the.mjs/.tsones also have a default export), in legacy, load and process modes, async and sync. All three file types must give the same values.Local verification
npm test, slow suitesnpm test, lib (src)npm test,testsnpm run typecheckGitHub Actions may be billing-blocked, so CI on this PR may not run.
Not changed:
varRegexinconfigorama/parse-file(src/utils/parsing/parse.js:98)The option is dead, but what it should do isn't defined anywhere, so it is left as is for a decision:
YAML.preProcess(fileContents, varRegex).preProcesshas never had a second parameter, though: it hardcodes${and only has a commented-outconsole.log(varRegex). df57855 (2025-12, "expose parse-file") added theregex = varRegex || DEFAULT_VAR_SYNTAXfallback. c6e9d98 (2025-12, "fix type errors") then dropped the argument from thepreProcesscalls. The option has never had any effect.index.d.tsdon't mention it. The follow-up note indocs/plans/onepassword-resolver-plugin.mdassumesDEFAULT_VAR_SYNTAXmatters for parse-file, but it doesn't.syntax: buildVariableSyntax('{{', '}}'), the README exampleapiKey: {{env:API_KEY}}parses as{"[object Object]": null}today, and[a, <env:X>]and[#{env:X}]fail.#,%,@,{,[) are YAML indicators.main.jsalso passesvarRegex: this.variableSyntax, so the change would alter YAML parsing on the main path for everysyntaxuser.Options: (A) implement regex-driven quoting in
YAML.preProcessfor non-${syntaxes, (B) remove the dead option andDEFAULT_VAR_SYNTAXand document that parse-file only knows${}, or (C) leave it.