ReviewOS

stacks/stx

Adoption report: a 6.5k-line production app hand-rolls router/data/forms/overlays despite the primitives being exported

#1843
Closed glennmichael123 opened this 11 days ago ·
11 days ago

Context

Feedback from bench, a ~6.5k-line production app built on stx + crosswind (a judge/review platform). I audited its client code to find every place it reaches past the framework into vanilla JS / raw DOM. The surprising result: nearly every workaround maps to an stx primitive that is already exported — so this is an adoption problem, not a missing-feature problem.

Filing it so (a) the delivery path can be verified against a released build, and (b) there's a migration story for existing apps.

Hack in the app → primitive that already exists

What the app hand-rolls today (evidence)Already-exported stx API
window.location.pathname.match(...) for route params (ArticleView.stx:78, JudgeDirectory.stx:26, ~20 files)useRoute · useRouteParams · useRouteParam
URLSearchParams + history.replaceState filter state (stores/search.ts:32)useSearchParams
window.location.assign() redirects (stores/auth.ts:108)navigate · redirect · goBack
hand fetch on mount + manual AbortController (34 components, every store)useFetch · useAsyncData · query
setInterval(fetch, 30000) notification polling (stores/notifications.ts:82)useEventSource · useSSE · useWebSocket · useChannel
per-store setTimeout debounce; dropdown blur race ×4 (stores/judges.ts:136, BenchHeader.stx:24)useDebounce · useClickOutside
getElementById().focus(); navigator.clipboard/share (SettingsView.stx:106, ProfileHeader.stx:100)useFocus · useClipboard · useShare
hand-built ToastHost/ConfirmHost signal-array portal (stores/toast.ts:15)StxToastBuiltin · StxModalBuiltin · Teleport
"can't call ref.show()" (stores/toast.ts:15-20)defineExpose
N signals + manual error flags + manual focus per form (ReviewForm.stx:90-254)defineForm · validateFields · defaultFormClasses
effect(() => { void a(); void b() }) watcher hack ×12 (ReviewForm.stx:337)watch · watchEffect · watchMultiple

Suspected cause

This app was written while the runtime-delivery layer was broken — the batch of now-closed issues where composables existed in source but never reached the client (#1804, #1805, #1819, #1832). The team correctly fell back to vanilla JS at the time and never migrated back.

Ask

  1. Confirm these composables reach the client runtime in a released (non-fork) build, via a small smoke app.
  2. Publish an adoption / migration guide (or codemod) for apps that hand-rolled these before the delivery fixes landed.
  3. If any still don't reach the client in a release, treat that as the residual bug.

Related: #1804 #1805 #1819 #1832 #1792

10 days ago

Audited the whole table against the runtime. Answering ask 1 with data, and ask 3 turned up one real gap — fixed in 5daaba1.

Ask 1 — what actually reaches the client

Of the 26 APIs named, 25 are delivered. Verified by booting the real runtime in a DOM and checking window.stx, plus the on-demand composable bundler for the browser-only ones:

On window.stx (17): useRoute · useSearchParams · navigate · goBack · useFetch · useWebSocket · useDebounce · useClickOutside · useFocus · watch · watchEffect · watchMultiple · defineExpose · plus useRouteParams · useRouteParam as of this fix.

Bundled on demand from src/composables (6): useAsyncData · useChannel · useClipboard · useEventSource · useSSE · useShare.

Ask 3 — the residual gap

useRouteParams and useRouteParam were not on window.stx, despite being exported from src/runtime.ts, whose docblock says it is "the public API for accessing stx runtime state, all window.* access abstracted away."

That matters more than one missing name: the bundler rewrites a client script's imports into a destructure from window.stx, so import { useRouteParams } from 'stx' yielded undefined and threw on call. The capability was always reachable via useRoute().params — only the documented names were not. That is exactly the shape of gap that sends someone back to window.location.pathname.match(...) and keeps them there, which is your ~20-file row.

Both are published now, and there is a surface test asserting that every API an app is told to import is a function on window.stx — so "it exists in src" can no longer pass for "it is delivered".

Four rows in the table are server-side by design

Not gaps, and shipping them to the browser would be the wrong fix:

namedactually
querydatabase.ts — server-side DB access
redirectedge-runtime.ts — edge/server redirect
validateFields, defaultFormClassesforms.ts — server-side directive processing ( and friends)

For those rows the client-side answers are useFetch / useQuery (already delivered), navigate (already delivered), and defineForm from forms-validation.ts for validation state.

Ask 2 — the migration guide

Not done, and worth being straight about why: most of it already exists, in the wrong place. The repo's CLAUDE.md carries a substantial "Reach for stx primitives before vanilla JS" section with a hack-to-primitive table that overlaps yours almost row for row — querySelector → ref(), addEventListener → useEventListener, localStorage.getItem → useLocalStorage, location.href → navigate, plain <a> → <StxLink>, and so on.

So the remaining work is publishing that as user-facing docs rather than writing it from scratch. That is a docs task with a real editorial decision in it (what belongs in an agent instruction file vs a migration guide), so I would rather you point it than have me guess. Happy to do it if you want it.

Note on "verify against a released build"

I verified against src. That is the right check for is the API delivered, but not for does a released tarball contain it — packages/stx/dist needs a rebuild before the next tag for #1832's fix in particular to reach consumers, since five dist entries each bundle their own copy of the runtime generator. Worth confirming on the next release rather than assuming.

Full suite 10811 pass in a clean worktree; the one failure is the pre-existing crosswind-arbitrary-values case.

10 days ago

Second data point for this thesis, from a different app — Postline, ~5.1k lines of .stx plus ~3.1k lines of browser TypeScript. I set out to find missing features and found mostly the same thing you did.

Rather than reading source, I checked which globals are actually assigned in our own built pages, with a known-negative control (window.thisDoesNotExist → 0) so the method is sound:

PrimitiveReaches our built pageTimes Postline uses it
toast, modal, draweryes0
stxConfirm, stxAlertyes0
useFetch, useQuery, watchyes0
useClickOutside, useFocusyes0
useSeoMetayes0
x-tooltip directiveyes0

Meanwhile the app calls bare confirm() in five places and uses 13 title= attributes where x-tooltip ships a positioned, styled tooltip runtime.

So: delivery is working in 0.2.153. This is adoption.

The discoverability mechanism, concretely

Two things make it very easy to never learn these exist:

1. The fallback is a working native API. confirm() resolves to the browser dialog. It works, so nothing ever prompts you to look for stxConfirm. Same for title= versus x-tooltip. A primitive whose absence is invisible doesn't get adopted — there's no failure to investigate.

2. Naming. My first search for the overlay primitives looked for stxToast / stxModal and returned zero. The globals are toast and modal. A wrong-name grep returning zero is indistinguishable from a missing feature, and I nearly filed "no toast service" on the strength of it. Given stxAlert and stxConfirm are prefixed, the inconsistent prefixing is itself a discoverability cost.

One real gap this surfaced

useForm genuinely is undiscoverable rather than merely unused. It ships and is implementable-against — @stacksjs/composables/dist/useForm.js, with per-field validation, inputProps() carrying aria-invalid/aria-describedby, isSubmitting, touched/dirty, and setErrors for 422 mapping. But:

runtime-globals.ts             useForm=0   (control useScrollLock=1)
browser-auto-imports.d.ts      useForm=0   (control useScrollLock=2)

Zero hits in both auto-import surfaces, against a control that hits in both. There is no way to find it without already knowing the package path.

Worth distinguishing from stx's own defineForm (forms-validation.ts:549), which is a validation schema library — its state is plain objects and isValid/isDirty are plain getters, so form.errors.email in a template never re-renders. Those are two different things with confusingly similar names, and only one of them can drive a template.

Suggestion on the ask

Adding these to the auto-import surface would do more than a migration guide, because it fixes discovery at the point of use rather than requiring someone to read a doc they don't know they need. A codemod for confirm( → stxConfirm( and title= → x-tooltip= would also be mechanical and safe.

I've filed the handful of things that survived verification as genuinely missing separately (#1882, #1883, #1884). Of 53 candidate gaps my agents proposed, 19 held up, 24 needed reframing, and 10 were wrong — the majority of the "missing" ones turned out to be exactly what this issue describes.

10 days ago

Items 1 and 2 done — but both "mechanical and safe" suggestions turned out not to be, so it is worth saying what changed and why.

Item 1 — useForm is reachable

Shipped in stacks as stacksjs/stacks#2271, not here: useForm lives in @stacksjs/composables, and stx's own use-form.ts exports defineForm — a different thing (see item 3 below). It is now re-exported from the browser vendors surface and declared in browser-auto-imports.d.ts. Both halves fail independently: without the re-export a bare useForm(...) is a ReferenceError; without the declaration no editor offers it, which is the failure that actually happened.

The control I used to measure this gap was itself broken. useScrollLock was declared in the generated surface while the module exported nothing of that name. And it is not one stale line:

declared from core/browser/src/utils/vendors : 243
actually exported by that module             :  12

The other ~231 are a VueUse-shaped surface — breakpointsTailwind, computedAsync, createEventHook — that an editor autocompletes and that resolves to nothing at runtime. The file carries @ts-nocheck, which is why nothing caught it. That is #1804's trap at scale, in stacks' browser surface, and it deserves its own issue rather than being stapled to a two-name fix. I fixed useForm and useScrollLock and left the rest measured but untouched.

Item 2 — stx codemod

Shipped here in 9fb6dce624. stx codemod reports hand-rolled sites and rewrites the ones it can prove safe; --fix writes, --rule narrows.

confirm( → stxConfirm( is not a safe rewrite. confirm() returns a boolean synchronously; stxConfirm() returns a Promise<boolean>. So:

if (confirm('Delete account?'))    { drop() }   // asks
if (stxConfirm('Delete account?')) { drop() }   // ALWAYS drops — a Promise is truthy

The suggested one-liner would have turned every confirmation into an unconditional yes. The rule now rewrites to await stxConfirm( only where it can see an enclosing async, and reports the rest with that reason attached. It also refuses a synchronous callback nested inside an async function, which is the same bug one level down.

title= → x-tooltip= is an accessibility regression. title is announced by screen readers; x-tooltip sets no role and no aria-* — it is a visual tooltip on mouseover/focusin. So the rule adds x-tooltip and keeps title:

<button title="Save now">          →  <button x-tooltip="Save now" title="Save now">

Making x-tooltip carry role="tooltip" + aria-describedby would make a true replacement possible. That is a runtime change and I have not done it — worth its own issue too, and arguably worth more than the codemod, since it would fix every future x-tooltip rather than the ones a migration touches.

The command never exits non-zero. A repo full of confirm() is not broken; it just has not adopted the primitive. Failing CI over that turns the tool into something people switch off.

24 tests, weighted toward the refusals.

Still open

Item 3 — the useForm / defineForm naming collision — is with the maintainer. They are different things with confusingly close names, and only useForm can drive a template: defineForm's state is plain objects, so form.errors.email in a template never re-renders. Nothing in this comment resolves that.

10 days ago

Item 3 settled: useForm is the name. Shipped in d4ab3c7808.

First, a correction to this thread

I repeated the earlier comment's claim that defineForm's state is plain objects, so form.errors.email never re-renders. That is stale — #1856 fixed it. It is one signal per field behind a proxy, with an explicit comment about why reactive() was avoided (#1885).

Which makes the situation worse, not better. The two are not "a schema builder and a reactive one". They are near-identical:

stx defineFormstacks useForm
values · errors · touched · dirty✅✅
validate · reset · handleSubmit · per-field accessor · setters✅✅
validating, validateField✅✗
isSubmitting, isValid, isDirty✗✅
setErrors (422), clearErrors✗✅
submitButtonProps, firstErrorField, aria inputProps✗✅

Same primitive, built twice, in two repos, because neither side could find the other's. That is this issue's own thesis — apps hand-roll what already exists — applied to the framework itself.

Why useForm and not something else

Not a taste call. The convention is already firm here: define* DECLARES (defineProps, defineEmits, defineExpose, definePageMeta, defineSlots, defineStore); use* returns live reactive state (useFetch, useCookie, useMutation, useDark). defineForm returned signal-backed state and a handleSubmit — the only member of the define family that did.

The file had always been use-form.ts, so grepping useForm found the filename with no export, and grepping defineForm found an export in a file named after something else. That is precisely how the audit above nearly filed "no form primitive".

What shipped

  • useForm is the export; defineForm is a deprecated alias for one minor — a plain export const defineForm = useForm, not a wrapper, so defineForm === useForm holds and there is no second implementation to drift.
  • Both spellings keep the same delivery path and are classified in NON_CLIENT_PRIMITIVES, so the #1846 guards still account for both.
  • The generated SERVER_ONLY_COMPOSABLES gained useForm — its test recomputes from the real module surface and caught the omission unprompted.
  • Test file renamed too; define-form-reactive.test.ts testing useForm would have been the same trap in miniature.
  • Docs updated.

376 tests across the affected suites, 9628 across stx + router, 0 related failures. (One pre-existing crosswind CSS failure, untouched by this.)

Deliberately not done

Making stacks' useForm a re-export of this one. It would couple stacks' form fixes to an stx release cadence, and the surfaces are near-identical but not equal — this one has validating/validateField, that one has isSubmitting/isValid/setErrors/submitButtonProps and the aria inputProps wiring. Merging means porting the union up and then deleting the duplicate, which is real work and a real coupling decision. Worth its own issue rather than a silent absorption.

So the naming is consistent now; the duplication is not resolved.

10 days ago

Ask 2 is done in a83305beb2, which closes the last item here.

stx codemod shipped with two rules against the eleven-row table in this report. It now covers the rest, plus an adoption guide linked from the sidebar.

The nine new rules report, they don't rewrite. The two existing ones can edit because the mapping is local and total. These can't: location.pathname.match(/\/judges\/(\d+)/) becomes useRouteParam('id') only if you know the route's parameter name, and a hand fetch + AbortController becomes useFetch only by restructuring the component around it. A tool that guessed would produce diffs nobody can review.

Reporting is most of the value regardless — the finding in this thread is that these primitives are delivered and used zero times, so the gap is knowing they exist, not typing the replacement.

An stx primitive already covers this — 4 site(s):
  views/x.stx:2  [route-params]  const id = location.pathname.match(/\/judges\/(\d+)/)
      use useRoute() / useRouteParams() / useRouteParam(name) — parsing the URL by hand
      re-implements the router and does not update on SPA navigation
  views/x.stx:3  [fetch]  const c = new AbortController()
      use useFetch(url) / useAsyncData(key, fn) — cancellation, loading and error state
      are already wired; hand-rolling them is where the races live

The report also separates a declined rewrite (needs a decision) from an adoption suggestion. Rolling them together made every finding look like a chore.

Two detectors are narrower than they look, and measurement is why. Running them over the 191-file example corpus turned up two false positives that would have taught people to ignore the tool:

  • setInterval(updateClock, 1000) was flagged as network polling, because the check asked whether the file fetches anywhere. It's local to the interval now.
  • location.href = 'mailto:…' was flagged as a missed navigate(), which cannot handle a mailto: at all.

Corpus after the fixes: 78 findings over 191 files, no false positive I can still find. Every "does not fire" case in the tests is one of these, kept so the narrowing can't be undone by accident.


Closing this, since all three asks are now settled:

  • Ask 1 — 25 of 26 APIs verified reaching a released client, then independently confirmed against a second app with a known-negative control.
  • Ask 3 — the one residual gap (useRouteParams / useRouteParam missing from window.stx) fixed in 5daaba1f9b.
  • Ask 2 — codemod + guide, above.

One thing worth carrying forward rather than losing here: the mechanism this report identified is that the fallback works. confirm() shows a dialog, title= shows a tooltip, location.href navigates. Nothing fails, so nothing prompts anyone to look for the framework version. That's why the remedy had to be a tool rather than a changelog entry — and it's the argument for running stx codemod in CI on the apps, rather than once at migration time.

Sign in to comment on this issue.