Uh oh!
There was an error while loading. Please reload this page.
docs(core): qualify both evaluateExpression references in the registerFunction JSDoc - #5649
Merged
Merged
Conversation
…terFunction JSDoc
`ExpressionEvaluator.ts` exports two entities spelled `evaluateExpression`: the
method on `ExpressionEvaluator` (bare expression, throws) and the module-level
export (context bag, fail-soft). The `registerFunction` doc block referred to
both under the one spelling, four lines apart.
Measured with the TypeScript checker, the unqualified `{@link evaluateExpression}`
did NOT bind to the method: it resolved to the module-level FunctionDeclaration —
the fail-soft one — while the sentence calls it "the throwing sibling". Sibling
`{@link evaluate}` binds to the method only because no module-level `evaluate`
exists to outrank it.
- prose link -> `{@link ExpressionEvaluator.evaluateExpression}`, which the
checker now resolves to the MethodDeclaration.
- the `@example`'s last line is marked as the module-level export and shows the
import it actually needs.
No behaviour change: the edit is confined to a block comment.
Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK…hecker
A `{@link}` that binds to the wrong entity is indistinguishable in source from
one that binds right, and nothing in this repo resolves link targets — no lint
rule, no runtime observation. The checker is the only thing that performs the
binding, and it is what an editor hover and the published `.d.ts` reader both go
through, so these cases ask it directly.
The program is created over the SOURCE file by relative path, so no build
artifact sits between an edit and a result.
Discrimination is built in rather than promised: one case recompiles the same
source with the single qualification removed and asserts the link lands back on
the module-level FunctionDeclaration, so the pin is measured to be capable of
failing on every run.
Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK…tion Scored `patch` rather than the empty-frontmatter form because the doc block is emitted into what npm ships: this edit moves both dist/evaluator/ExpressionEvaluator.d.ts and dist/evaluator/ExpressionEvaluator.js, the package building with a bare `tsc` that preserves comments in the JS emit. 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 22, 2026 01:08
Uh oh!
There was an error while loading. Please reload this page.
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#5580
ExpressionEvaluator.tsexports two entities spelledevaluateExpression— the method onExpressionEvaluator(bare expression, throws) and the module-level export (context bag, fail-soft). TheregisterFunctiondoc block referred to both under the one spelling, four lines apart. Both references are now qualified.Premise re-verified — and one of the card's claims is measured FALSE
The card's
:142(method) still holds. Its:368for the module-level export was stale: PR #5578 added ~120 lines of JSDoc above it, so the export now sits at:405. Re-derived onaa3b81062.The larger correction is to the card's central claim. It states the prose
{@link evaluateExpression}"resolves, per TSDoc, to the method. Correct.", and concludes "Not a defect — every statement in that block is true." Asked of the TypeScript checker (checker.getSymbolAtLocation) on the pre-fix source::405is the module-level export — the fail-soft one — inside the sentence that calls it "the throwing sibling, and reports'formatCurrency' is not a function." So the block did contain a false statement; this is a small real defect, not pure imprecision.Note why the neighbouring link is not evidence of the opposite:
{@link evaluate}binds to the method only because no module-levelevaluateexists to outrank it. "An unqualified link in a class member resolves to that class's member" is false here, and that is precisely the reading the card relied on.After the fix:
The fix
{@link ExpressionEvaluator.evaluateExpression}, which the checker resolves to theMethodDeclaration.@example's last line is the module-level export (context bag as second parameter; the${...}wrapper only resolves on theevaluatepath), but it sat two lines under calls establishingevaluator.as the receiver, and a.d.tshover carries no import to disambiguate. It now names the module-level export and shows the import it needs. BothevaluateExpressionandExpressionEvaluatorare exported from the package entry, so that import is true as written (verified against the built entry).Checkable, not asserted — and a declared surface expansion
The dispatch asked that the corrected bindings be made checkable, since a
{@link}that binds to the wrong entity is indistinguishable in source from one that binds right. Nothing in this repo resolves link targets: no lint rule, nothing at runtime. The only thing that performs the binding is the checker — which is what an editor hover and the published.d.tsreader both go through.So this PR adds one file beyond the dispatched surface, declared in the same round on the issue before it was committed:
packages/core/src/evaluator/__tests__/registerFunction-jsdoc-links.test.tsIt creates a program over the source file by relative path, so no build artifact sits between an edit and a result. It follows the existing house pattern for compiler-driven pins (
packages/core/src/utils/__tests__/freeze-schema.types.test.ts), including that precedent's built-in discrimination leg. Bindings are asserted askind+ owning class rather than line numbers, so the pin does not rot the way the card's:368did.The pin can fail — measured on its own mutation leg, twice
In-suite (every run): one case recompiles the same source with the single qualification removed, via an in-memory host overlay, and asserts the link lands back on the module-level
FunctionDeclaration.External ablation: the source was reverted to
origin/mainand the suite re-run.Two red, and the direction was predicted before the run: the in-suite ablation case stays green under external ablation, because with the source already bare the mutated copy equals the original and the bare link still resolves to the function. It is stated here rather than counted as evidence.
Mutation was confirmed on disk by anchored
grep -cFon both the removed and injected text, never an editor exit code — anchors asserted pristine (1 / 0) before mutating, then inverted (0 / 1) after. The script carriedtrap ... EXIT INT TERM; the tree was verified clean afterwards.Shipped bytes — the
.jsprediction is falsifiedThe prediction under test was:
dist/**/*.d.tsmoves,dist/**/*.jsbyte-identical. Measured over all 90 + 90 emitted files, clean-built both legs (dist/andpackages/core/tsconfig.tsbuildinfocleared — the build info lives outsidedist/, so clearingdistalone would have skipped emit).dist/evaluator/ExpressionEvaluator.d.ts8eb97d48…e194d74948e971…0fa3ba08dist/evaluator/ExpressionEvaluator.js478dcb80…3a4a4f141fcb9ffa…9cfbd5a1Exactly one file moved on each leg; the other 89 + 89 are unchanged. The
.jsmoves because this package builds with a baretsc, which preserves comments in the JS emit — so JSDoc reaches both published artefacts, not only declarations.The intent behind the prediction (no program semantics change) still holds, and is proven directly rather than by
--removeComments, which proves nothing about semantics: the emitted.jsdelta is 10 changed lines, of which 0 are non-comment lines.Determinism was cross-checked in both directions: the pre-fix rebuild reproduced the original baseline hashes exactly, and the restore rebuild reproduced the post-fix hashes exactly — so the movement is attributable to this edit, not to build noise, and no mutated artefact was left in
dist.The new test file does not enter the published emit: dist counts stay 90 / 90, with no
__tests__directory and no artefact bearing its name.Changeset
check-changeset-presence.mjsis the authority and was run rather than inferred from the.d.tsresult — it exits 1 before, 0 after. Scoredpatch, nevermajor(fixed group), matching PR #5578's reasoning on this identical surface: the block is emitted into what npm ships, and the measurement above makes that stronger than #5578 knew, since the JSDoc reaches the.jsas well.Verification
All at
3de60fdcd, the head of this branch. Exit codes captured before any pipe; each gate's own verdict line quoted.pnpm exec vitest run packages/core/CORE_VITEST_EXIT=0—Test Files 94 passed (94)·Tests 1950 passed (1950)pnpm exec vitest run packages/core/src/evaluator/EVALUATOR_VITEST_EXIT=0—Test Files 12 passed (12)·Tests 364 passed (364)VITEST_EXIT=0—Tests 5 passed (5)pnpm --filter @object-ui/core type-checkTYPECHECK_EXIT=0(tsc --noEmit && tsc -p tsconfig.test.json)pnpm --filter @object-ui/core lintLINT_EXIT=0—✖ 513 problems (0 errors, 513 warnings)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-fixed.mjsEXIT=0—✅ All workspace packages are in the changeset fixed group.node scripts/check-changeset-no-major.mjsEXIT=0—✅ No changeset declares amajorbump.node scripts/check-control-bytes.mjsEXIT=0—✅ OK (scanned 4697 tracked text file(s); skipped 85 binary)node scripts/check-package-self-import.mjsEXIT=0—✅ No package names itself inside its own src/.check:self-importis called out deliberately: the new@exampleline containsimport { evaluateExpression } from '@object-ui/core';insidepackages/coreitself. The gate reports0 self-importacross16132 module specifier(s), so a specifier in a comment is not counted.The test suite total moved
93 -> 94files and1945 -> 1950tests against PR #5578's figures — exactly the one file and five cases added here, and nothing else.Repo-wide
pnpm lint/pnpm testare left to CI, and the narrowing is declared rather than assumed. Three pieces, since a narrowing without them is just a skipped run:packages/core.--format json, not estimated. Both changed source files are in it; the new test file contributes0 errors, 0 warnings, and the totals (0 errors, 513 warnings) are the pre-existing baseline unchanged.eslint.config.jsdeclares noprojectServiceand noparserOptions.project, so linting is not type-aware and this diff cannot move a verdict in any file it does not itself contain.check:published-distwas not run to completion — it hit the container's 10-minute foreground cap. Its concern for this diff is answered directly above (dist counts unchanged at 90 / 90, no__tests__indist); the full repo-wide scan belongs to CI. Known-broken gauges left untouched:check:eager-closure(exits 2),check:doc-snippet-types(exits 1) — the latter's population is.md/.mdxand package READMEs, so JSDoc@exampleblocks in.tsare outside it either way.Scope
Exactly the dispatched surface plus the one declared test file. The fence held:
packages/core/src/actions/**(#5611's open draft) is untouched. No behaviour change, no declaration moves — the source diff is confined to a block comment.Generated by Claude Code