fix(pages-router): router.push() resolves to true on success#1566
Open
james-elicx wants to merge 1 commit into
Open
fix(pages-router): router.push() resolves to true on success#1566james-elicx wants to merge 1 commit into
james-elicx wants to merge 1 commit into
Conversation
Pages Router `window.next.router.push`/`replace` already resolved to `true` on a successful navigation (and `false` on a hard-navigation fallback) via `performNavigation`, but the public `window.next.router` surface typed both as `(...args: unknown[]) => unknown`, leaving consumers (and the Next.js deploy test suite that `awaits` the result) without a typed boolean to assert on. - Narrow `PagesRouterPublicInstance.push`/`replace` to `(...args: unknown[]) => Promise<boolean>` to match Next.js's `Router.change` contract (router.ts:1206 — `Promise<boolean>`). - Add unit-test regression coverage asserting `window.next.router.push`/`replace` resolve to `true` on success. - Void the `router.push(...)` calls in `tests/e2e/app-router/build-id-navigation.spec.ts` to satisfy the tightened union (App Router push is `void`). Closes #1467
Member
Author
|
/bigbonk review |
commit: |
|
Contributor
There was a problem hiding this comment.
Clean, minimal fix. The type tightening, void annotations, and regression tests all look correct.
Verified:
performNavigationexhaustively returnstrueorfalseon every non-throwing path (lines 916, 939, 948, 949, 960 inrouter.ts), so thePromise<boolean>annotation is accurate.push/replaceinRouterMethodsdirectlyreturn performNavigation(...)with no intermediate processing — no path can resolve toundefined.- The
void router.push(target)in the E2E spec is the right fix: App Router push returnsvoid, Pages Router returnsPromise<boolean>, so the union isvoid | Promise<boolean>.voidsuppresses the floating-promise lint without changing behavior. - Tests cover both
pushandreplacevia shallow navigation, which is sufficient to exercise the return-value contract without needing a fullnavigateClientstub. - Test cleanup (
finallyblock restoringwindow+vi.resetModules()) matches the established pattern in the surroundingwindow.nexttest block.
LGTM.
Contributor
|
Approved PR #1566. The change is straightforward and correct:
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1467
Summary
router.push/router.replacealready resolved totrueon success andfalseon a hard-navigation fallback viaperformNavigation. This PR pins down the contract on the publicwindow.next.routersurface so consumers (and the Next.js deploy test suite thatawaits the return value) get a typedPromise<boolean>instead ofunknown.await window.next.router.push("/foo")andawait window.next.router.replace("/foo")resolve totrue.router.push(...)calls intests/e2e/app-router/build-id-navigation.spec.tsto satisfy the tightened union (App Router push isvoid).Test plan
vp test run tests/shims.test.ts -t "window.next.router.push resolves to true"(added test)vp test run tests/shims.test.ts(974 passed)vp test run tests/pages-router.test.ts(241 passed)vp test run tests/router-javascript-urls.test.ts(21 passed)vp check(no warnings or errors)