Skip to content

fix: drop submitter-needs-bound-form, which contradicts #1307 - #1385

Merged
vivek7405 merged 3 commits into
mainfrom
fix/drop-submitter-needs-bound-form
Aug 10, 2026
Merged

fix: drop submitter-needs-bound-form, which contradicts #1307#1385
vivek7405 merged 3 commits into
mainfrom
fix/drop-submitter-needs-bound-form

Conversation

@vivek7405

@vivek7405 vivek7405 commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Closes #1384

Summary

Removes the submitter-needs-bound-form check rule and one false assertion in the scanner test. Both were written against renderer behaviour that does not exist, and both have reddened main since 7683c1ba, blocking every open PR including #1383.

#1307 made a bound submitter self-sufficient. The renderer stamps formmethod="post" and formenctype on the button itself, and per HTML a submitter's formmethod overrides the form's method. So a bound button posts correctly inside a form that is unbound, method-less, or even method="get". Measured against render-server.js at 034d6a0e:

<form method="get"><button formaction=${publish}>x</button></form>
  -> <form method="get"><button name="__webjs_action" value="…"
             formmethod="post" formenctype="multipart/form-data">x</button></form>

The rule's premise, stated in its own source comment, was that such a submission "defaults to GET, the identity rides the query string, and the page re-renders with the action never having run". That is false for every shape it flagged. In practice it was flagging #1307's own demonstration fixtures in examples/blog, one of which carries a doc comment explaining why it works. The differential test asserted a rejection matching /requires the enclosing <form> to also be bound/, a string that exists nowhere in packages/core/src or packages/server/src.

What changed

  • Removed the rule from packages/server/src/check.js: the RULES entry, the call site, and checkSubmitterNeedsBoundForm() (about 440 lines), plus the now-unused scanHtmlFormScopes import.
  • Deleted packages/server/test/check/submitter-needs-bound-form.test.js.
  • Removed only the false assert.rejects block from html-form-scopes.test.js, keeping the scanner assertions around it and retitling the test.
  • Corrected four doc surfaces that stated the enclosing form is what supplies method="post".

Deliberately not done

  • The runtime diagnostics in form-dispatch.js stay. WEBJS_FORM_SUBMITTED_AS_GET and WEBJS_FORM_ACTION_MISSING detect a request SHAPE server-side and remain reachable however the request was produced (a hand-written form, an explicit formmethod="get" the renderer honours on a plain submitter, a stale cached page). test/bun/form-action-dispatch.mjs covers them and passes. The docs now say what actually reaches the first one.
  • examples/blog is untouched. publish-button.ts and triage-split/page.ts are feat: make a bound form submitter self-sufficient, and honour the authored enctype #1307's fixtures. Editing them to satisfy the rule would have inverted cause and effect and deleted the proof that the feature works.
  • scanHtmlFormScopes() is deleted too. It was added by feat: resolve form-submitter boundness in webjs check and make the residual loud #1314 solely to feed this rule, so removing the rule left it with no caller. Review then found it was not merely unused: its delivers field answers false for a form with no method, which is exactly the shape that now works, so the scan was wrong and its tests pinned the wrong values. The two live surfaces are salvaged into packages/server/test/scanner/form-action-holes.test.js: classifyActionHole, which the surviving form-action-not-a-get-action rule uses, and the enctype pin that keeps the renderer allowlist and the client guard's denylist from drifting.
  • matchClosingBrace's fix from feat: resolve form-submitter boundness in webjs check and make the residual loud #1314 is kept. It is a real bug fix (brace depth was incremented at a ${ hole and never decremented) that five check rules and the elision analyser depend on.

Test plan

  • webjs check passes on examples/blog, gallery and website, 0 violations each, with the blog files unmodified. Verified against the WORKTREE's own check.js: a linked worktree resolves bare @webjsdev/* to the primary checkout, so the first run silently tested the unmodified rule and still reported the violations.
  • packages/server/test/scanner/html-form-scopes.test.js 18/18 on Node
  • Same file 18/18 under bun test. It was the sole genuine Bun-matrix failure on main, and it is gone from the failure list.
  • node scripts/run-bun-tests.js 310 pass, 27 documented node-only skips
  • node --test 'test/docs/*.test.mjs' 27/27
  • website boots in prod mode: /, /docs/troubleshooting, /docs/progressive-enhancement and /ui all 200 with no broken modulepreload

Two local failures are NOT from this diff, each confirmed by re-running at unmodified origin/main in the primary checkout:

  • packages/server/test/elision/differential-elision.test.js:177 fails in the linked worktree, passes 9/9 in the primary.
  • test/bun/listener.test.mjs fails in the worktree AND the primary, so it is pre-existing and local.

Neither can trace here: check.js is imported by nothing in the server request path, so this diff cannot affect rendering or the listener.

Layers that do not apply: browser and e2e (no routing, DOM, or client-router surface changes; the diff is a static analyser plus prose).

The rule and its renderer differential were written against behaviour the
renderer does not have. #1307 made a bound submitter self-sufficient: the
renderer stamps formmethod="post" and formenctype on the button itself, and
a submitter's formmethod overrides the form's method per HTML. So a bound
button posts correctly inside an unbound, method-less, or method="get"
form, and the rule's premise that it "submits as a GET with the identity in
the query string" is false for every shape it flagged.

It was flagging #1307's own demonstration fixtures in examples/blog, and
the differential asserted an error string that exists nowhere in core or
server. Both have reddened main since 7683c1b, on webjs check, the unit
suite, and the Bun matrix, blocking every open PR.

The runtime diagnostics in form-dispatch.js are untouched: they detect a
request shape server-side and stay reachable however the request was made.
@vivek7405 vivek7405 self-assigned this Aug 10, 2026
Review of the removal turned up five places still asserting, or still
built for, the enclosing-form question #1307 deleted.

The gallery's submit-todo comment contradicted itself four lines apart,
saying the enclosing form does NOT have to be bound and then that it does,
and pointed readers at the rule this branch removes. That file is scaffold
payload, so it ships into every generated app.

The production onError message for WEBJS_FORM_SUBMITTED_AS_GET named the
enclosing form as the cause; the dev logger four lines above it was already
correct, so the two disagreed on one event. render-server.js still carried
the four-state form-scope comment and passed a vestigial third argument to
a two-parameter render(), both left over from the same removal. check.js
kept two imports whose only uses were inside the deleted function, and the
now-callerless scanner's JSDoc still described the consumer it lost.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went over the whole diff. The removal is right, and I checked the renderer directly rather than taking the rule's word for it: a bound submitter really does come out carrying its own formmethod="post" and formenctype in an unbound form, in a method="get" form, and with no form at all. So the rule was asserting a failure mode that cannot happen, and it was flagging the very fixtures #1307 added to prove that.

What I'd watch is that the premise outlived the rule in more places than the rule itself. Five surfaces still described, or were still built for, the enclosing-form question. Findings, all fixed in ec59af0c, anchored path-level because the fix deleted the lines they sat on:

  1. packages/server/src/check.js kept matchClosingParenthesis and extractComponents imported with no call sites left. Their only uses were inside the deleted function.
  2. gallery/modules/todo/actions/submit-todo.server.ts contradicted itself inside one comment block, saying the enclosing form does NOT have to be bound and then, twelve lines later, that it does. It also pointed readers at the removed rule. That file is scaffold payload, so it ships into every generated app.
  3. packages/server/src/form-dispatch.js named the enclosing form as the cause in the production onError message for WEBJS_FORM_SUBMITTED_AS_GET, while the dev logger four lines above it already gave the real one. Two messages for one event, disagreeing.
  4. packages/core/src/render-server.js still carried the four-state form-scope comment and passed a vestigial third argument to a two-parameter render(). Both are leftovers from the same #1307 removal, in the file this PR cites as its evidence.
  5. packages/server/src/js-scan.js had scanHtmlFormScopes JSDoc describing a caller that no longer exists.

One thing left deliberately: scanHtmlFormScopes has no production caller now, since #1314 added it purely to feed this rule. I kept it and marked the JSDoc honestly rather than deleting another 250 lines plus a 330-line test file in a PR whose job is unblocking main. Worth a follow-up decision, not worth widening this diff.

The second review round found the scan is not merely callerless. Its
`delivers` field encodes the premise this branch removes:
`unboundFormDelivers` answers false for a form with no method or a
method="get" one, but a bound submitter now carries its own
formmethod="post", which overrides the form, so those forms DO deliver.
Its tests asserted the wrong values, and the blog fixture it would score
false is the one the e2e proves runs its action.

Salvaged the two live surfaces into form-action-holes.test.js:
classifyActionHole, which the form-action-not-a-get-action rule still
uses, and the enctype pin that keeps the renderer allowlist and the client
denylist from drifting.

Also cleared four more residuals of the same premise: the onError message
claimed a bound submitter's formmethod="get" is honoured when it is
refused at render, the renderer comment said a submission targets the page
rather than whatever the form targets, the streaming machine kept an
orphaned form-scope comment, and three doc surfaces called a refused
method="get" on a bound form a warning.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second pass, scoped to the previous round's fixes and their blast radius. It turned up the thing the first pass and I both missed, and it changes a call I had made.

scanHtmlFormScopes is not merely callerless. Its delivers field encodes the exact premise this branch removes: unboundFormDelivers answers false for a form with no method or a method="get" one, but a bound submitter now carries its own formmethod="post", which overrides the form, so those forms do deliver. Its tests asserted the wrong values, and the shape it would have scored false is examples/blog/app/feedback/triage-split/page.ts, whose action the e2e proves runs. So "unused but correct, keep it" was wrong, and I deleted it rather than documenting a lie. The two live surfaces are salvaged into form-action-holes.test.js: classifyActionHole, which the form-action-not-a-get-action rule still uses, and the enctype pin that keeps the renderer allowlist and the client guard's denylist from drifting.

Four more residuals of the same premise, all fixed:

  1. packages/server/src/form-dispatch.js still told readers to look for a formmethod="get" on the pressed button. On a BOUND submitter that is refused at render, so the advice pointed at something that cannot exist. It now names a plain submitter, which is the case that actually reaches the diagnostic.
  2. packages/core/src/render-server.js said the submission "still targets the page". It targets whatever the FORM targets, which is what invariant 12 and the gallery comment say. It also justified an early refusal by the enclosing form's boundness, which nothing computes any more.
  3. packages/core/src/render-server.js streaming machine kept an orphaned form-scope comment dangling over an unrelated declaration, the same residual I cleaned in the buffered machine and missed in its twin.
  4. SKILL.md, data-and-actions.md and muscle-memory-gotchas.md all called method="get" on a bound form a WEBJS_FORM_SUBMITTED_AS_GET warning. It is a thrown render error. Two of those files were edited by the earlier commit, so the stale claim survived a pass over the same lines.

Stopping the cycle here. The remaining risk is low: everything in this round was a comment, a message, a doc line, or the removal of code with no caller, and the renderer's observable output is unchanged across all three form shapes.

@vivek7405
vivek7405 marked this pull request as ready for review August 10, 2026 20:37
@vivek7405
vivek7405 merged commit 6a3044e into main Aug 10, 2026
10 checks passed
@vivek7405
vivek7405 deleted the fix/drop-submitter-needs-bound-form branch August 10, 2026 20:41
vivek7405 added a commit that referenced this pull request Aug 10, 2026
The rebase onto #1385 dropped submitter-needs-bound-form from check/, but
three comments still carried its premise: the render-client reconciler said
a submitter asks whether its enclosing form is bound, the DSD pass still
documented the 'unknown' form scope it no longer passes, and the check
runner named the rule as a sharer of classifyActionHole.
vivek7405 added a commit that referenced this pull request Aug 10, 2026
…1386)

Clears every package carrying unreleased user-facing work since 0.7.49.
intellisense and the two editor packages picked up nothing in the range, so
they stay where they are.

core takes the two attribute-reader fixes. A custom converter.fromAttribute
never ran during SSR, so an element painted one value and held another the
moment it upgraded, and the SSR reader resolved attribute names with its own
resolver rather than the set observedAttributes delivers, which disagreed with
the browser on four shapes of hand-written markup. Both readers now share one
implementation. Each carries a behaviour note in its entry: a converter that
touches a browser global now throws server-side, and three attribute shapes
read LESS at SSR to match what the browser already did.

server takes the boot-time validation of the `webjs` config block, so a typo'd
key warns once instead of silently costing a feature its setting, plus the
auto-linking of app/icon and app/apple-icon, which closes the gap between
shipping the Next-shaped file convention and never referencing it. The fixes
cover the headers rules dropped in silence and the production message for a
form submission arriving as a GET, which named a cause #1307 had made
impossible.

cli emits one cross-agent instruction set from `webjs create` instead of a rule
file per tool, sources the scaffold gallery from the repo's runnable gallery
app (bundled into the tarball at pack time, verified with npm pack), and
refuses `webjs check` outside an app, where every rule's one-application
premise does not hold.

mcp resolves the docs corpus from the app being edited rather than the
snapshot frozen into a global install, and says which one it served.

ui stops two registry modules doing work at module scope, which cost page
elision in every app using the kit, and teaches cn() the Tailwind v4
parenthesis hint spelling.

Raises packages/server's declared @webjsdev/core range from ^0.7.49 to
^0.7.50. No new core export is imported statically, but the release PR is the
only place that bump is legal, and core carries the earliest date in the batch
so it publishes first.

The generated notes were curated before committing. #1314 is dropped from
server entirely: it added the submitter-needs-bound-form check rule that #1385
then removed in the same range, so the rule never reaches a release, and what
remains is the diagnostic message the fix entry describes. Slices that were a
docblock, a test, or an export keyword are dropped from the packages that saw
only those.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: drop submitter-needs-bound-form, which contradicts the self-sufficient bound submitter

1 participant