fix: next/form submitter overrides and query-string GET URLs#426
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: next/form submitter overrides and query-string GET URLs
Good bug fix overall — the submitter handling and appendSearchParamsToUrl utility are well-structured, and the test coverage (unit + E2E) is thorough. A few things to flag:
Behavioral divergence from Next.js on query string handling
The most significant concern: Next.js strips existing query params from the action URL before appending form data, matching the HTML spec for URL-encoded GET form submissions. vinext now preserves them via appendSearchParamsToUrl.
From Next.js form-shared.tsx:
if (targetUrl.searchParams.size) {
targetUrl.search = '';
}Next.js also emits a dev warning: <Form> received action that contains search params: "...". This is not supported, and they will be ignored.
This means the new action="/search?lang=en" pattern in the test fixtures would be warned against by Next.js and the lang=en param would be dropped. vinext preserving it is arguably more useful, but it's a behavioral difference worth documenting or deciding on intentionally. If you want to match Next.js, the existing query should be cleared, and the test expectations adjusted. If you want to intentionally diverge here (reasonable), a brief code comment explaining the choice would help future contributors.
Other notes below as inline comments.
| nativeEvent && | ||
| typeof nativeEvent === "object" && | ||
| "submitter" in nativeEvent && | ||
| nativeEvent.submitter instanceof Element |
There was a problem hiding this comment.
The instanceof Element check here uses the global Element. In the unit tests this works because FakeElement is stubbed as Element. In real browsers this is fine too.
However, there's a subtle difference from Next.js: Next.js accesses event.nativeEvent.submitter directly without instanceof checks — the browser's SubmitEvent.submitter is already typed as HTMLElement | null. The defensive instanceof checking is fine for robustness, but worth noting that Next.js doesn't guard this way.
| submitter: FormSubmitter | null, | ||
| formMethod: FormHTMLAttributes<HTMLFormElement>["method"], | ||
| ): string { | ||
| const override = submitter?.getAttribute("formmethod"); |
There was a problem hiding this comment.
Lowercase "formmethod" is correct for getAttribute() — HTML attributes are case-insensitive and the DOM normalizes them to lowercase. Good.
One behavioral note: Next.js doesn't read the effective method this way. Instead it uses hasUnsupportedSubmitterAttributes() which bails out entirely if formMethod is anything other than "get". The practical result is similar (non-GET submissions fall through to native browser behavior), but the Next.js approach also blocks formEncType and formTarget overrides that aren't supported. Consider whether you want to add those guards too.
| } | ||
|
|
||
| const url = `${action as string}?${params.toString()}`; | ||
| const url = appendSearchParamsToUrl(effectiveAction, params); |
There was a problem hiding this comment.
This is the key behavioral divergence point. The old code was ${action}?${params} which was buggy (double ? if action already had a query). The fix to use appendSearchParamsToUrl is correct for the merging bug, but it preserves existing query params.
Next.js explicitly clears existing search params before appending form data (matching the HTML spec for URL-encoded GET submissions). A dev-mode warning about query params in the action URL would bring this closer to parity.
If preserving is the intended behavior, a code comment here explaining the intentional divergence would be valuable.
| * Append query parameters to a URL while preserving any existing query string | ||
| * and fragment identifier. | ||
| */ | ||
| export function appendSearchParamsToUrl(url: string, params: Iterable<[string, string]>): string { |
There was a problem hiding this comment.
Clean implementation. The manual string parsing (instead of new URL(url, 'http://x')) avoids issues with relative paths and query-only URLs like ?lang=en. Good call.
Minor edge case to consider: URLSearchParams constructor will decode + as space in the existing query string, then toString() will re-encode spaces as +. This is correct behavior, but worth being aware that existing %20-encoded spaces in the URL would be re-encoded as + after round-tripping through URLSearchParams. Probably fine for all practical cases.
| }; | ||
| } | ||
|
|
||
| function renderClientForm(props: Record<string, unknown>) { |
There was a problem hiding this comment.
This renderClientForm helper reaches into the component's internal .render method directly. This is a pragmatic approach for unit testing the submit handler without a full DOM, but it's fragile — it depends on forwardRef exposing a .render property, which is a React implementation detail that could change.
Not blocking, but worth a comment explaining why this approach was chosen over something like @testing-library/react.
| return class FakeFormData implements Iterable<FormEntry> { | ||
| private entries: FormEntry[] = []; | ||
|
|
||
| constructor(form?: FormTarget, submitter?: FakeSubmitterElement | null) { |
There was a problem hiding this comment.
Nit: the FakeFormData constructor receives submitter typed as FakeSubmitterElement | null, but the real FormData constructor accepts HTMLElement | null. The test's supportsSubmitter: false path throws unconditionally when submitter !== undefined, which means passing null explicitly would also throw. In real browsers, new FormData(form, null) does not throw — only truly unsupported environments (old browsers without the two-arg overload) would throw.
This is fine for the specific test cases here since you only pass null implicitly (no submitter arg), but the fake doesn't perfectly model the real API.
| await disableViteErrorOverlay(page); | ||
| } | ||
|
|
||
| async function installPageLoadCounter(page: import("@playwright/test").Page) { |
There was a problem hiding this comment.
Nice improvement over the window.__FORM_MARKER__ approach. addInitScript + sessionStorage is more reliable for detecting full-page reloads since addInitScript runs on every page load (including navigations that create a new JS context), while sessionStorage persists across navigations within the same tab. Good pattern.
| </button> | ||
| </Form> | ||
|
|
||
| <Form action="/search?lang=en" id="search-form-with-query"> |
There was a problem hiding this comment.
As noted in the top-level comment: in Next.js, action="/search?lang=en" would trigger a dev-mode warning and the lang=en param would be stripped during form submission. The recommended Next.js pattern is to use a hidden input instead:
| <Form action="/search?lang=en" id="search-form-with-query"> | |
| <Form action="/search" id="search-form-with-query"> | |
| <input type="hidden" name="lang" value="en" /> | |
| <input name="q" placeholder="Search with locale..." id="search-input-with-query" /> |
If you decide to intentionally support query-string preservation (diverging from Next.js), the current approach is fine — but the test fixture should note this is a vinext extension.
| const method = getEffectiveMethod(submitter, rest.method); | ||
| if (method !== "GET") return; | ||
|
|
||
| const effectiveAction = getEffectiveAction(submitter, action as string); |
There was a problem hiding this comment.
Good: you re-run isSafeAction on the effective action after resolving submitter overrides. This prevents a safe form from being redirected to an unsafe URL via a submitter's formAction. The e.preventDefault() on line 136 blocks the submission entirely rather than falling through to native behavior — this is the right call for a security guard.
|
The review has been posted. Here's a summary of the feedback I provided: Key finding: Behavioral divergence from Next.js. Next.js explicitly strips existing query params from the form action URL before appending form data (matching the HTML spec for URL-encoded GET submissions), and emits a dev warning when query params are present in the action. This PR preserves them via Inline comments covered:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: next/form submitter overrides and query-string GET URLs
This is a solid PR. The implementation correctly matches Next.js behavior (stripping query params from action URLs and warning in dev), the submitter handling is well-structured, and both unit and E2E test coverage are thorough. The appendSearchParamsToUrl utility is clean and its reuse in link.tsx and router.ts is a nice improvement.
A few issues to address, one of which is a real bug:
Bug: Case mismatch in hasUnsupportedSubmitterAttributes
The function uses mixed-case getAttribute calls ("formEnctype", "formMethod", "formTarget") but getEffectiveMethod and getEffectiveAction use lowercase ("formmethod", "formaction"). In the real DOM, getAttribute is case-insensitive so both work. However, the unit tests' FakeSubmitterElement.getAttribute stores attributes by exact key and does a fallback to name.toLowerCase() — this works, but only because the test attributes are already lowercase. The inconsistency is confusing and could cause bugs if someone adds a test with camelCase attribute keys. More importantly, the HTML spec says attribute names are lowercased in the DOM, so all getAttribute calls should use lowercase for consistency.
Other notes inline.
| return true; | ||
| } | ||
|
|
||
| const formMethod = submitter.getAttribute("formMethod"); |
There was a problem hiding this comment.
Same case inconsistency:
| const formMethod = submitter.getAttribute("formMethod"); | |
| const formMethod = submitter.getAttribute("formmethod"); |
| form: HTMLFormElement, | ||
| submitter: FormSubmitter | null, | ||
| ): string { | ||
| const targetUrl = new URL(action, window.location.href); |
There was a problem hiding this comment.
Nit: this new URL(action, window.location.href) call will throw if window.location.href is somehow invalid (shouldn't happen in a real browser, but the unit tests stub window.location.href as a plain string). The code is fine in practice since checkFormActionUrl already ran with a try/catch, but it's worth noting this function assumes a valid base URL.
Also — nice that you strip search params here (line 137-138) to match Next.js form-shared.tsx behavior. This is the right call.
| if (e.defaultPrevented) return; | ||
| } | ||
|
|
||
| const submitter = getSubmitter(e.nativeEvent); |
There was a problem hiding this comment.
Good: submitter resolution and unsupported-attribute checks happen before the method !== "GET" early return. This means a POST form with a submitter that has formTarget="_blank" correctly falls through to native behavior rather than silently ignoring the submitter. Correct ordering.
| function renderClientForm(props: Record<string, unknown>) { | ||
| // `forwardRef()` exposes the wrapped render function on `.render`, which lets us | ||
| // exercise the submit handler directly without adding a DOM renderer just for this shim. | ||
| const rendered = (Form as unknown as { render: (props: Record<string, unknown>) => any }).render( |
There was a problem hiding this comment.
As noted in the existing review: this relies on forwardRef exposing a .render property, which is a React implementation detail. React 19 still has this, but it's not part of the public API.
The comment on line 83-84 documents the rationale well. If React ever removes this, the test will fail with a clear error, so it's low risk. Fine for now.
| await disableViteErrorOverlay(page); | ||
| } | ||
|
|
||
| async function installPageLoadCounter(page: import("@playwright/test").Page) { |
There was a problem hiding this comment.
Nice improvement. The addInitScript + sessionStorage pattern is more robust than the previous window.__FORM_MARKER__ approach, since addInitScript runs on every navigation (including full page loads) while sessionStorage persists across navigations within the same tab. The count accurately reflects the number of full page loads.
| * Append query parameters to a URL while preserving any existing query string | ||
| * and fragment identifier. | ||
| */ | ||
| export function appendSearchParamsToUrl(url: string, params: Iterable<[string, string]>): string { |
There was a problem hiding this comment.
Clean implementation. Using manual string parsing rather than new URL() is the right call here — it avoids issues with relative paths, query-only URLs like ?lang=en, and doesn't require a base URL.
Note that appendSearchParamsToUrl is now used in link.tsx and router.ts resolveHref but not in form.tsx (which uses createFormSubmitDestinationUrl and strips query params). This separation is correct — forms should strip per the HTML spec, while resolveHref for object-style hrefs should preserve since the developer explicitly constructed the pathname with a query string.
| </button> | ||
| </Form> | ||
|
|
||
| <Form action="/search" id="search-form-with-hidden-lang"> |
There was a problem hiding this comment.
Good: the previous bonk review flagged the potential divergence from Next.js around query params in actions. I see this fixture now uses hidden inputs instead, which matches the Next.js-recommended pattern. The form action is clean (/search) and the lang=en value comes from a hidden <input>. This is correct.
| @@ -0,0 +1,19 @@ | |||
| import SearchForm from "../search/search-form"; | |||
There was a problem hiding this comment.
Minor: this page imports SearchForm but doesn't really need it for the test assertions (the E2E tests never interact with the forms on the /search-alt page — they just verify the result text). Including it is fine for realism though, and it doesn't hurt.
|
The review has been posted. Here's a summary of what I flagged: Bug found: Case mismatch in Positive findings:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: next/form submitter overrides and query-string GET URLs
This is well-structured work. The two bugs (malformed GET URLs with existing query strings, ignored submitter overrides) are real, and the fix correctly matches Next.js behavior — stripping query params from the action URL with a dev warning, honoring formAction/formMethod/submitter name=value, and re-checking isSafeAction on the effective action after submitter resolution.
The appendSearchParamsToUrl utility is clean, and reusing it in link.tsx and router.ts for object href resolution is a nice consolidation. The separation between form submission (strips query params) and resolveHref (preserves them) is correct.
Test coverage is thorough — unit tests cover the submit handler directly (including the FormData submitter fallback path), and E2E tests exercise the full navigation flow including cross-page submitter formAction.
A few minor items inline, none blocking.
| targetUrl.searchParams.append(name, typeof value === "string" ? value : value.name); | ||
| } | ||
|
|
||
| return toSameOriginPath(targetUrl.href) ?? targetUrl.href; |
There was a problem hiding this comment.
Nit: toSameOriginPath returns pathname + search + hash from a URL object. Since you just cleared targetUrl.search and rebuilt it via searchParams.append, this works correctly. But if action originally had a fragment (e.g., /search?q=1#section), the fragment would survive targetUrl.search = "" and appear in the destination URL. Is that intentional?
Next.js also preserves the hash in this path (form-shared.tsx doesn't strip it), so this is fine — just worth a comment if the behavior is intentional, since fragments in form actions are unusual.
| } | ||
|
|
||
| const submitter = getSubmitter(e.nativeEvent); | ||
| if (submitter && hasUnsupportedSubmitterAttributes(submitter)) { |
There was a problem hiding this comment.
Good: hasUnsupportedSubmitterAttributes returns early (falls through to native browser behavior) before the method !== "GET" check. This means a POST form with an unsupported formEnctype on its submitter correctly gets native handling rather than silently ignoring the attribute.
One edge case worth considering: if a submitter has formMethod="POST" (unsupported, triggers the error), the early return means the form never reaches e.preventDefault(), so the browser handles it natively. That's correct — but the console.error message says "This will disable
| function renderClientForm(props: Record<string, unknown>) { | ||
| // `forwardRef()` exposes the wrapped render function on `.render`, which lets us | ||
| // exercise the submit handler directly without adding a DOM renderer just for this shim. | ||
| const rendered = (Form as unknown as { render: (props: Record<string, unknown>) => any }).render( |
There was a problem hiding this comment.
This .render property on forwardRef components is a React implementation detail. It works in React 18 and 19, but it's not a public API. A brief comment about what to do if this breaks (e.g., switch to @testing-library/react or react-dom/test-utils) would help future contributors.
Not blocking — the existing comment on lines 85-86 explains the "why" well.
| return windowStub; | ||
| } | ||
|
|
||
| afterEach(() => { |
There was a problem hiding this comment.
The afterEach at module scope means it runs after every test in the file, including the SSR tests that don't stub globals. That's harmless (restoring mocks that don't exist is a no-op), but it would be slightly cleaner to move this into the "Form client GET interception" describe block so the cleanup is co-located with the setup.
| merged.append(key, value); | ||
| } | ||
|
|
||
| const search = merged.toString(); |
There was a problem hiding this comment.
Minor: when params is empty and existingQuery is non-empty, this still round-trips the existing query through URLSearchParams. That means %20 in the original URL becomes + in the output (both are valid, but it's a subtle normalization). The test on line 38-41 of query.test.ts covers the empty-params case, but only with simple values — might be worth a test with %20 to document this behavior.
Not blocking, just a heads-up for anyone debugging URL encoding differences.
| submitter?: FakeSubmitterElement | null; | ||
| }) { | ||
| const event = { | ||
| currentTarget: { entries }, |
There was a problem hiding this comment.
The currentTarget is { entries } — a plain object with an entries property. This works because FakeFormData's constructor reads form?.entries. In a real browser, currentTarget would be an HTMLFormElement and new FormData(form) would collect all input values from the DOM.
This is fine for testing the submit handler logic, but means the unit tests can't catch bugs where FormData collection from real DOM elements behaves differently (e.g., disabled fields, unnamed inputs). The E2E tests cover that gap, so this is acceptable.
|
Review posted. Here's a summary of my feedback: Verdict: Approved -- this is solid work that fixes two real bugs with thorough test coverage. Key observations:
Minor inline notes (none blocking):
|
|
Comments look fine. |
2 participants
Fix
next/formGET submissions so they preserve existing query strings and honor the clicked submitter's overrides.This fixes two concrete bugs:
actionbuilt malformed URLsformAction,formMethod, and submittername=valuewere not appliedWhat changed
Updated
next/formto:formActionformMethodname=valuein submitted form dataFormData(form, submitter)is unavailableAdded a shared URL helper for safe query param merging and reused it in:
next/formnext/linkobject href resolutionTests
Added/updated coverage for:
next/formsubmit interception behaviorformActionformMethod="GET"overriding a POST formname=valueLinkhrefs that already contain query strings and hashesVerification
Ran:
pnpm test tests/form.test.ts tests/query.test.ts tests/link.test.ts tests/shims.test.ts -t "next/form|Form|appendSearchParamsToUrl|Link resolveHref"PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e tests/e2e/app-router/form.spec.ts --repeat-each 4pnpm run fmtpnpm run typecheckpnpm run lintpnpm run buildpnpm test