Uh oh!
There was an error while loading. Please reload this page.
docs(core): state the registerFunction case-fold on the method itself - #5578
Merged
Merged
Conversation
…#5363) `ExpressionEvaluator.registerFunction` delegates to `FormulaFunctions.register`, which stores under `name.toUpperCase()`, so `registerFunction('formatCurrency', fn)` registers `FORMATCURRENCY` and only that spelling resolves inside `${...}`. The fold is deliberate — the built-in formula vocabulary is spreadsheet-style (`SUM`, `IF`, `UPPER`) and goes through the same call — but it was declared nowhere on the public method, and two mechanisms hide it: the registry API stays case-insensitive (`has`/`get` answer to the original spelling), and a wrong-case call site soft-fails through `evaluate()`'s catch to `defaultValue ?? expression`, rendering the template's own source as literal text instead of raising. Documentation only; behavior is unchanged. Three test cases pin the previously uncovered half so that making registration case-preserving fails a test rather than silently invalidating the JSDoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-sales
marked this pull request as ready for review
August 21, 2026 14:51
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 21, 2026
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 freeto 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.
Fixes#5363
What this is — and what it deliberately is not
The card reads like a bug:
ExpressionEvaluator.registerFunction('formatCurrency', fn)registers
FORMATCURRENCY, so${formatCurrency(price)}renders its own${...}sourceon screen. This PR changes no behavior. It states the case-fold on the method that
performs it, so the declaration matches what the code enforces, and pins the half of that
contract nothing covered.
I re-derived the fork rather than taking it on trust, and agreed with triage: direction 1.
Reasoning is in the report comment on #5363; the short form is that the fold is the
registry's contract, not an accident of this method —
FormulaFunctions.registerfolds,and every built-in (
SUM,IF,UPPER) is registered through the same call. Making onlyregisterFunctioncase-preserving would put two dialects in one registry, which is exactlywhat commandment #0.1 exists to stop, and it is a breaking change rather than an
unobserved one:
ActionRunner.scriptAwait.test.tsalready registers'doWrite'and callsDOWRITE(). That is a Feature card through the decision inbox, not a drive-by, so I did notwrite it.
Premise re-verified on today's
mainThe issue measured against
bdf8cf76e;origin/mainis nowac73c24b0. Re-measured againsta fresh
packages/core/dist/build — the premise holds, and one detail is sharper than thecard states:
evaluate('${formatCurrency(price)}')after registering'formatCurrency''${formatCurrency(price)}'— the source, verbatimevaluate('${FORMATCURRENCY(price)}')'$1,234.50'getFormulas().has('formatCurrency')/.get(...)true/function— case-insensitivegetFormulas().toObject()keyFORMATCURRENCYevaluateExpression('formatCurrency(price)')"formatCurrency" is not a functionThe sharper detail: the registry API is case-insensitive (
has/getanswer to theoriginal spelling). Only expressions are case-sensitive, because the evaluation scope is
built from
toObject()— a plain object whose identifiers match exactly. That is why thefold is invisible right up until someone writes an expression, and it is the part the new
JSDoc leads with.
The pin can fail — measured, not asserted
The dispatch asked for a pin that proves something about the documented contract, or none
at all. Registering
'DOUBLE'and callingDOUBLE(...)proves nothing, so the three newcases pin the uncovered half: that the given spelling does not resolve, that the failure
renders raw template source instead of raising, and that the registry API stays
case-insensitive underneath.
Reverse-verified by ablation —
FormulaFunctions.registermutated toset(name, fn), i.e.direction 2 simulated:
All three new cases go red; the 79 pre-existing ones stay green — so nothing previously
covered this fold at all, and the pin is a real tripwire on the behavior change this card
declined. Mutation was confirmed on disk by anchored
grep -con both the removed and theinjected text (1→0 and 0→1) plus
git diff --stat, never an editor exit code; the scriptcarried a
trap ... EXIT INT TERMrestore, and the tree was verified byte-identical to HEADafterwards (
git status --porcelainempty). The suite resolves its subject by relativesource path, so no
distis in the loop and no rebuild sits between mutation and result.Changeset: a real
patch, not the empty-frontmatter exemption.changeset/README.mdsays not to write changesets for documentation updates. That rule isabout documentation files; this is source in a released package, and the JSDoc is emitted
into the published artifact — measured:
So it is what consumers see on hover, which is a user-visible change to what npm ships.
Cheap to overrule: swap the frontmatter for the empty form.
Verification
Exit codes captured before any pipe; each gate's own verdict line quoted.
pnpm --filter @object-ui/core lintLINT_EXIT=0—✖ 513 problems (0 errors, 513 warnings), all pre-existingno-explicit-anypnpm --filter @object-ui/core type-checkTYPECHECK_EXIT=0(tsc --noEmit && tsc -p tsconfig.test.json)pnpm exec vitest run packages/core/src/evaluator/__tests__/ExpressionEvaluator.test.tsTest Files 1 passed (1)·Tests 82 passed (82)pnpm exec vitest run packages/core/(whole package)Test Files 93 passed (93)·Tests 1945 passed (1945)node scripts/check-control-bytes.mjsEXIT=0—✅ OK (scanned 4629 tracked text file(s))node scripts/check-changeset-presence.mjsEXIT=0—✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjsEXIT=0—✅ No changeset declares a major bumpnode scripts/check-changeset-fixed.mjsEXIT=0—✅ All workspace packages are in the changeset fixed groupAll run at
29d23acc0, which is the head of this branch.Repo-wide
pnpm lint/pnpm type-checkare left to CI, and the narrowing is declaredrather than assumed. Evidence, since a narrowing without it is just a skipped run:
eslint's own population for
packages/coreis 185 files (count read from--format json,not guessed), both touched files are in it; and root
eslint.config.jsdeclares noprojectService/parserOptions.project, so linting is not type-aware and this diffcannot move a verdict in any file it does not itself contain. For
type-checkthe invarianceis stronger still: the
ExpressionEvaluator.tsdiff contains zero non-comment lines(single hunk at line 349; lines 1–200 byte-identical to
origin/main), so the emitted typesignature is unchanged and no consumer package can be affected.
One measurement worth flagging:
eslint . --no-inline-configreports 5 errors inpackages/core, but that flag is not what this repo's gate runs (pnpm lintisturbo run lint→ per-packageeslint .). All 5 sit at deliberateeslint-disablecomments in code this PR does not touch — including the ES2020
preserve-caught-errorwaiver at
ExpressionEvaluator.ts:176, proven pre-existing above.Scope
Exactly the dispatched surface, no breach:
ExpressionEvaluator.ts, one test file, onechangeset.
FormulaFunctions.tscarries the same fold undocumented on its ownregister,but it is outside the declared surface and its
toUpperCase()is visible in its one-linebody — noted in the report rather than filed or fixed.
Generated by Claude Code