Repository navigation
refactor(configorama): isomorphic simplify pass (net -211 LOC) - #86
Merged
Merged
Conversation
Imports that nothing reads (tsc --noUnusedLocals, TS6133/TS6198): - main.js: os, find-up, variableSpans - metadata.js: splitOnPipe - parsers/typescript.js: path, fs - resolvers/valueFromFile.js: path, resolveFilePath, encodePathIdentity, and the in-branch requires of executeTypeScriptFile / executeESMFile (loadExecutable requires those parsers itself) - utils/encoders/literal-braces.js: setOwn - utils/parsing/enrichMetadata.js: resolveInnerVariables - utils/ui/configWizard.js: fs, path - utils/variables/cleanVariable.js: findNestedVariables, functionRegex, fileRefSyntax (only named in a commented-out block) ## Change: remove unread bindings (P12) - Inputs covered: all callers; no code path reads these names - Ordering preserved: yes. Where a module is no longer required from a file, it has only declarations at top level (no load-time effects) and its require closure does not reach back to the importing file, so no module sees a different partially-initialized export. find-up is still loaded by getFullFilePath when needed - Tie-breaking: N/A - Error semantics: unchanged; every dropped module exists and loads without throwing - Laziness / short-circuit: unchanged (a few modules now load later or not at all) - Floating-point / RNG / hash order: N/A - Observable side-effects: none - Type narrowing: N/A; exports unchanged, emitted .d.ts tree identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped), as baseline; npm run typecheck 0 errors; tsc declaration output identical to d2a254e; goldens (configorama() over 275 tests/ fixtures, plain and returnMetadata modes, results/errors plus stdout/stderr) identical.
- resolvers/valueFromGit.js: _exec (shell exec), superseded by _execFile for every git call; not exported - utils/strings/bracketMatcher.js: findEnclosingMultiChar; not exported, no callers - utils/variables/cleanVariable.js: removeOuterMostBraces and its findOutermostBraces wrapper (plus that wrapper's bracketMatcher require); referenced only from a commented-out line, which goes too Dead-code checks for each: no source, dynamic, string, test, build, plugin or doc references (grep over the whole monorepo); not in any module.exports; package `exports` exposes none of them; last touched in the monorepo move, not WIP/scaffold commits. ## Change: delete unreachable private helpers - Inputs covered: none reach them - Ordering / tie-breaking / laziness / short-circuit: N/A - Error semantics: unchanged (bodies never ran). bracketMatcher is no longer required at cleanVariable load; it has only declarations at top level and its require closure does not reach cleanVariable - Floating-point / RNG / hash order: N/A - Observable side-effects: none - Type narrowing: N/A; exports unchanged, emitted .d.ts identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts output identical to d2a254e; fixture goldens identical.
Unread locals (tsc --noUnusedLocals), each with a side-effect-free initializer: - display.js: `sourceType` (iterate Object.values instead of entries) and `typeText` (a chalk.dim string nobody used) - main.js: module const `logLines`; `variablesKnownTypes`, `hasFilters`, `v` and overwrite()'s `propertyString` (plain property reads/compares) - metadata.js: `finalFoundVariables` and the `foundVariables` array it was built from (only ever pushed to) - enrichMetadata.js: two `varKey`s (iterate Map values instead of entries) - PromiseTracker.js: `delta` (Date.now() arithmetic) - literal-braces.js: decodeForDisplay's `decodeJsonInVariable` and the `jsFixes` cache that held it (decoding goes through opaque.decodeAll) - configWizard.js: `isRequired` from a destructuring Left alone: parse.js `regex` / DEFAULT_VAR_SYNTAX. It is unread too, but docs/plans/onepassword-resolver-plugin.md plans to fix it, and the documented `varRegex` option it was meant to honor is a real gap. ## Change: remove unread locals - Inputs covered: every path through those functions - Ordering preserved: yes. Object.values / Map.values iterate in the same order as entries(), with the same values - Tie-breaking: N/A - Error semantics: unchanged. The removed reads cannot throw: in overwrite() and populateVariable() valueObject is dereferenced before (splitAndGet calls valueObject.value.match first); `this` reads are plain data properties; Date.now/chalk/Set have no failure modes here - Laziness / short-circuit: decodeForDisplay no longer loads js-fixes; that module has only declarations at top level - Floating-point / RNG / hash order: N/A - Observable side-effects: none (no logs, writes or I/O removed) - Type narrowing: N/A; exports unchanged, emitted .d.ts identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts identical to d2a254e; fixture goldens identical; CLI goldens (111 fixtures x `--info`, `inspect`, `--format yaml`, `--verify`, stdout+stderr+exit code) identical to d2a254e.
…ppers
ini.js, toml.js, json5.js and yaml.js wrap each of their 4 functions as
`let x; try { x = expr } catch (e) { throw new Error(e) } return x`
(16 sites). Each now returns `expr` from inside the try; the catch is
unchanged.
## Change: inline the temporary in 16 try/rethrow wrappers
- Inputs covered: parse/dump/toYaml/toJson/toToml of all four parsers
- Ordering preserved: yes; same single expression, same nesting (e.g.
toYaml still wraps parse's already-wrapped error a second time)
- Tie-breaking: N/A
- Error semantics: identical; the same expression throws inside the
same try and is rethrown as `new Error(e)`. yaml.parse's `= {}`
initializer was only overwritten or skipped by the throw, never
returned
- Laziness / short-circuit: unchanged (lazy requires still inside try)
- Floating-point / RNG / hash order: N/A
- Observable side-effects: none
- Type narrowing: emitted .d.ts identical (inferred return types same)
Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0
errors; .d.ts identical to d2a254e; fixture goldens identical; property
check of every parser function over 577 inputs (valid/invalid INI,
TOML, JSON5, YAML incl. tests/_fixtures files; dump of objects, null,
undefined, Date, functions, cycles): results and error messages
identical to d2a254e.
renderConfigWizard prompted `self` references and `dotProp` references
with two byte-identical blocks that differed only in the group key and
how a name is shown in the note (`${self:name}` vs `${name}`); Type II
clone, ~35 lines each. They now run as one loop over
[{ group: 'self', ... }, { group: 'dotProp', ... }].
## Change: loop over the two config-reference groups
- Inputs covered: every wizard run with self and/or dot.prop variables
- Ordering preserved: yes; self group first, then dotProp; within a
group the same note, then one prompt per variable in order
- Tie-breaking: N/A
- Error semantics: identical; same p.cancel + `Setup cancelled` throw
on a cancelled prompt; an empty group is skipped as before
- Laziness / short-circuit: unchanged (prompts awaited one at a time)
- Floating-point / RNG / hash order: N/A
- Observable side-effects: same p.note / p.text / p.password /
p.cancel calls with the same arguments; answers land in
userInputs.self / userInputs.dotProp as before
- Type narrowing: N/A; exports unchanged, emitted .d.ts identical
Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0
errors; .d.ts identical; fixture goldens identical; wizard golden: 522
runConfigWizard runs (174 fixtures' metadata x {answer, empty answer,
cancel on 3rd prompt}) against a recording fake @clack/prompts, every
call, argument, validator result and return value identical to
d2a254e (55 runs hit the Config References groups).
renderConfigWizard ended with four identical `if (Object.keys(x).length === 0) delete x` blocks for options, env, self and dotProp (Type II, 4 sites). They are now one loop over those keys in the same order. ## Change: loop over section names - Inputs covered: every completed wizard run - Ordering preserved: yes; options, env, self, dotProp, so the returned object's remaining keys keep their insertion order - Tie-breaking: N/A - Error semantics: unchanged; no throws - Laziness / short-circuit: N/A - Floating-point / RNG / hash order: N/A - Observable side-effects: same deletes on the returned object - Type narrowing: N/A; emitted .d.ts identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts identical; wizard golden (522 runs incl. returned userInputs) identical to d2a254e.
The per-path map in displayVariableDetails returned the same
`${prefix}${pathLink}${typeText}` from inside the per-occurrence-type
branch and again after it (Type I clone). The branch now only sets
typeText and falls through to the shared return.
## Change: drop the duplicated prefix/return inside the branch
- Inputs covered: variables referenced from more than one config path
- Ordering preserved: yes; typeText is assigned before the return in
both paths, as before (the outer `typeText` the box title reads is
still overwritten per path, last one wins)
- Tie-breaking: N/A
- Error semantics: unchanged; no throws
- Laziness / short-circuit: N/A
- Floating-point / RNG / hash order: N/A
- Observable side-effects: identical console output
- Type narrowing: N/A; emitted .d.ts identical
Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0
errors; .d.ts identical; CLI goldens identical; display golden: the
three display* functions called on 174 fixtures' metadata (24 with
multi-path variables) with captured console output identical to
d2a254e.
…branches In getDeeperValue's get mode, the `subProperty !== '' || '' in object` branch and the following `isString(reducedValue)` branch had byte-identical bodies (try JSON.parse, then index by subProperty). They are now one branch whose condition ORs the two tests. ## Change: `else if (A) X else if (B) X` -> `else if (A || B) X` - Inputs covered: every deep-reference step that is not build mode and not an undefined value - Ordering preserved: yes - Tie-breaking: N/A - Short-circuit eval: same sequence; the old else-if chain evaluated `subProperty !== ''`, then `typeof === 'object' && '' in value`, then isString only if both were false, exactly as `||` does. The `'' in null` TypeError case is reached at the same point as before - Error semantics: unchanged (same expressions, same swallowed JSON.parse failure) - Laziness: unchanged - Floating-point / RNG / hash order: N/A - Observable side-effects: none - Type narrowing: N/A; emitted .d.ts identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts identical; fixture goldens (incl. deep-reference fixtures) identical.
- main.js (Fn::Sub with verifySubReferences): four else-if branches (resource ref, parameter ref, ApiGatewayRestApi, HttpApi) each only set `allowSpecialCase = true`; now one condition ORing the four tests - cli.js: the requirements branch and the audit/graph/inspect branch both set `inputFile = argv._[1] || null`; now one condition Found with an AST scan for sibling if-branches with identical bodies. ## Change: `if (A) X else if (B) X ...` -> `if (A || B ...) X` - Inputs covered: Fn::Sub references to resources, parameters, the two serverless hidden names and unknown names; every CLI subcommand - Ordering preserved: yes - Short-circuit eval: same test order (resources, params, then the two names); later tests run only when earlier ones are falsy, as in the else-if chain - Tie-breaking: N/A - Error semantics: unchanged; same "not found" error for unknown names - Laziness: unchanged - Floating-point / RNG / hash order: N/A - Observable side-effects: none - Type narrowing: N/A; emitted .d.ts identical; CLI flags unchanged Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts identical; fixture and CLI goldens identical; property check of Fn::Sub handling (9 names incl. __proto__/constructor x 4 config shapes x verifySubReferences true/false/unset x disableDefaultIgnorePaths true/false = 216 cases) identical to d2a254e.
valueFromFile handled ts/tsx/mts/cts and mjs/esm references with two
blocks that were identical apart from the label used for
processModule's fileType and the "Error processing <label> file"
message (Type II clone). One block now runs with the label picked from
the extension.
## Change: merge the two executable-module branches
- Inputs covered: ${file()} refs to .ts/.tsx/.mts/.cts/.mjs/.esm,
with and without :exports, loading OK or throwing
- Ordering preserved: yes; same position in the if-chain (after the JS
branch), parseModuleReference before loadExecutable, as before
- Tie-breaking: an extension maps to at most one label (disjoint sets)
- Error semantics: identical messages ("Error processing TypeScript
file: ..." / "Error processing ESM file: ..."), still a rejected
promise from the same try/catch
- Laziness / short-circuit: unchanged
- Floating-point / RNG / hash order: N/A
- Observable side-effects: same module loads
- Type narrowing: N/A; emitted .d.ts identical
Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0
errors; .d.ts identical; fixture goldens identical; ad-hoc check of 14
refs (good/broken/throwing .ts and .mjs, default/named/missing exports)
gives identical values and error messages to d2a254e.
…Frontmatter The +++, --- and <!-- frontmatter branches differed only in the opening line, closing marker and whether the format is fixed (TOML) or detected (Type II clone, 3 sites). They are now one loop over FRONTMATTER_DELIMITERS: [open, close, fixedFormat]. ## Change: parameterize three delimiter branches by a table - Inputs covered: every markdown/MDX file passed to extractFrontmatter - Ordering preserved: yes; same check order (+++, ---, <!--). The three openings are mutually exclusive prefixes anyway - Tie-breaking: N/A - Error semantics: unchanged; no throws (falsy input still returns noMatch first) - Index arithmetic: indexOf starts at open.length (4, 4, 5), body is sliced from open.length, content from endIdx + close.length (4 for all three), the same literals as before - Laziness: detectSyntax still runs only for --- and <!-- (fixed 'toml' short-circuits the ||) - Floating-point / RNG / hash order: N/A - Observable side-effects: none; returned object has the same keys in the same order - Type narrowing: N/A; exports unchanged, emitted .d.ts identical Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0 errors; .d.ts identical; fixture goldens identical; property check of extractFrontmatter over 150,000 random strings built from delimiters, CRLF, TOML/JSON/YAML snippets, plus '' and undefined: identical to d2a254e.
The .js/.cjs branch of parseFileContents wrapped its body in
`try { ... } catch (err) { throw err }`, which rethrows the same value
untouched (P1). The body now runs without the wrapper; `jsFile` becomes
a const.
## Change: remove a no-op try/catch
- Inputs covered: every .js/.cjs config file (module, function export,
throwing at load, throwing in the export, throwing a non-Error)
- Ordering preserved: yes; same statements in the same order
- Tie-breaking: N/A
- Error semantics: identical; the thrown value (Error or not) reaches
the caller as the same object, and its stack was captured where it
was created, not at the rethrow
- Laziness / short-circuit: unchanged
- Floating-point / RNG / hash order: N/A
- Observable side-effects: none
- Type narrowing: N/A; emitted .d.ts identical
Verification: npm test 8+27+863(4 skipped)+1810(1 skipped); typecheck 0
errors; .d.ts identical; fixture goldens identical; ad-hoc .js configs
(object export, function that throws, module that throws a string)
give identical results/errors to d2a254e.
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.
Isomorphic simplify pass on
packages/configorama/src(2026-10-24). Public API, CLI and error messages unchanged; emitted.d.tsidentical. Net -211 LOC (19 files, +76/-287). No version bump._exec,findEnclosingMultiChar,removeOuterMostBraces)parseFileContentsVerified:
npm test(8 + 27 + 863 + 1810/1810, 1 skipped),npm run typecheck0 errors, fuzz (fixed seed) green; goldens identical to d2a254e (275 fixtures, 111 CLI fixtures x 4 flag sets, 522 wizard runs).Bugs noticed (not fixed):
parse.js:98varRegexis computed and unused;.mjs/.tssyntax errors via${file()}resolve to{ __JITI_ERROR__ }instead of throwing (esm.js:29, loadExecutable.js:16);selectModuleExport(valueFromFile.js:493) rejects named non-function exports of.mjs/.ts.