Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/next/src/build/webpack/plugins/define-env-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export function getDefineEnv({
isEdgeServer ? 'edge' : isNodeServer ? 'nodejs' : ''
),
'process.env.NEXT_MINIMAL': JSON.stringify(''),
'process.env.__NEXT_WINDOW_HISTORY_SUPPORT': JSON.stringify(
Comment thread
timneutkens marked this conversation as resolved.
config.experimental.windowHistorySupport
),
'process.env.__NEXT_ACTIONS_DEPLOYMENT_ID': JSON.stringify(
config.experimental.useDeploymentIdServerActions
),
Expand Down
149 changes: 126 additions & 23 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
PrefetchKind,
} from './router-reducer/router-reducer-types'
import type {
PushRef,
ReducerActions,
RouterChangeByServerResponse,
RouterNavigate,
Expand Down Expand Up @@ -108,24 +109,44 @@ function isExternalURL(url: URL) {
return url.origin !== window.location.origin
}

function HistoryUpdater({ tree, pushRef, canonicalUrl, sync }: any) {
function HistoryUpdater({
tree,
pushRef,
canonicalUrl,
sync,
}: {
tree: FlightRouterState
pushRef: PushRef
canonicalUrl: string
sync: () => void
}) {
useInsertionEffect(() => {
// Identifier is shortened intentionally.
// __NA is used to identify if the history entry can be handled by the app-router.
// __N is used to identify if the history entry can be handled by the old router.
const historyState = {
...(process.env.__NEXT_WINDOW_HISTORY_SUPPORT &&
pushRef.preserveCustomHistoryState
? window.history.state
Comment thread
timneutkens marked this conversation as resolved.
: {}),
// Identifier is shortened intentionally.
// __NA is used to identify if the history entry can be handled by the app-router.
// __N is used to identify if the history entry can be handled by the old router.
__NA: true,
tree,
__PRIVATE_NEXTJS_INTERNALS_TREE: tree,
}
if (
pushRef.pendingPush &&
// Skip pushing an additional history entry if the canonicalUrl is the same as the current url.
// This mirrors the browser behavior for normal navigation.
createHrefFromUrl(new URL(window.location.href)) !== canonicalUrl
) {
// This intentionally mutates React state, pushRef is overwritten to ensure additional push/replace calls do not trigger an additional history entry.
pushRef.pendingPush = false
window.history.pushState(historyState, '', canonicalUrl)
if (originalPushState) {
originalPushState(historyState, '', canonicalUrl)
}
} else {
window.history.replaceState(historyState, '', canonicalUrl)
if (originalReplaceState) {
originalReplaceState(historyState, '', canonicalUrl)
}
}
sync()
}, [tree, pushRef, canonicalUrl, sync])
Expand Down Expand Up @@ -204,6 +225,28 @@ function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
)
}

const originalPushState =
typeof window !== 'undefined'
? window.history.pushState.bind(window.history)
: null
const originalReplaceState =
typeof window !== 'undefined'
? window.history.replaceState.bind(window.history)
: null

function copyNextJsInternalHistoryState(data: any) {
const currentState = window.history.state
const __NA = currentState?.__NA
if (__NA) {
data.__NA = __NA
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@timneutkens I believe data can be very plausible be null or undefined here based on the replaceHistory API.

}
const __PRIVATE_NEXTJS_INTERNALS_TREE =
currentState?.__PRIVATE_NEXTJS_INTERNALS_TREE
if (__PRIVATE_NEXTJS_INTERNALS_TREE) {
data.__PRIVATE_NEXTJS_INTERNALS_TREE = __PRIVATE_NEXTJS_INTERNALS_TREE
}
}

/**
* The global router that wraps the application components.
*/
Expand Down Expand Up @@ -371,12 +414,16 @@ function Router({
// would trigger the mpa navigation logic again from the lines below.
// This will restore the router to the initial state in the event that the app is restored from bfcache.
function handlePageShow(event: PageTransitionEvent) {
if (!event.persisted || !window.history.state?.tree) return
if (
!event.persisted ||
!window.history.state?.__PRIVATE_NEXTJS_INTERNALS_TREE
)
return

dispatch({
type: ACTION_RESTORE,
url: new URL(window.location.href),
tree: window.history.state.tree,
tree: window.history.state.__PRIVATE_NEXTJS_INTERNALS_TREE,
})
}

Expand Down Expand Up @@ -416,13 +463,66 @@ function Router({
use(createInfinitePromise())
}

/**
* Handle popstate event, this is used to handle back/forward in the browser.
* By default dispatches ACTION_RESTORE, however if the history entry was not pushed/replaced by app-router it will reload the page.
* That case can happen when the old router injected the history entry.
*/
const onPopState = useCallback(
({ state }: PopStateEvent) => {
useEffect(() => {
if (process.env.__NEXT_WINDOW_HISTORY_SUPPORT) {
// Ensure the canonical URL in the Next.js Router is updated when the URL is changed so that `usePathname` and `useSearchParams` hold the pushed values.
const applyUrlFromHistoryPushReplace = (
url: string | URL | null | undefined
) => {
startTransition(() => {
dispatch({
type: ACTION_RESTORE,
url: new URL(url ?? window.location.href),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: This should be based on window.location.href, to allow relative/partial URLs.

One can set search params on the current page (and maintain router state) via history.replaceState(history.state, '', '?foo=bar'), but this line fails as it's not a valid URL.

suggestion: new URL(url ?? '', window.location.href)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I created issue #56636 some time ago. I spotted this experimental feature in the 14.0.3 release notes and thought it could fix it. However, when I try it, I just get TypeError: Failed to construct 'URL': Invalid URL errors. It looks to be related to this review comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks @franky47. I just tested against 14.0.4-canary.1 which includes #58438 and it looks like it will now resolve #56636.

tree: window.history.state.__PRIVATE_NEXTJS_INTERNALS_TREE,
})
})
}

if (originalPushState) {
/**
* Patch pushState to ensure external changes to the history are reflected in the Next.js Router.
* Ensures Next.js internal history state is copied to the new history entry.
* Ensures usePathname and useSearchParams hold the newly provided url.
*/
window.history.pushState = function pushState(
data: any,
_unused: string,
url?: string | URL | null
): void {
copyNextJsInternalHistoryState(data)

applyUrlFromHistoryPushReplace(url)

return originalPushState(data, _unused, url)
}
}
if (originalReplaceState) {
/**
* Patch replaceState to ensure external changes to the history are reflected in the Next.js Router.
* Ensures Next.js internal history state is copied to the new history entry.
* Ensures usePathname and useSearchParams hold the newly provided url.
*/
window.history.replaceState = function replaceState(
data: any,
_unused: string,
url?: string | URL | null
): void {
copyNextJsInternalHistoryState(data)

if (url) {
applyUrlFromHistoryPushReplace(url)
}
return originalReplaceState(data, _unused, url)
}
}
}

/**
* Handle popstate event, this is used to handle back/forward in the browser.
* By default dispatches ACTION_RESTORE, however if the history entry was not pushed/replaced by app-router it will reload the page.
* That case can happen when the old router injected the history entry.
*/
const onPopState = ({ state }: PopStateEvent) => {
if (!state) {
// TODO-APP: this case only happens when pushState/replaceState was called outside of Next.js. It should probably reload the page in this case.
return
Expand All @@ -441,20 +541,23 @@ function Router({
dispatch({
type: ACTION_RESTORE,
url: new URL(window.location.href),
tree: state.tree,
tree: state.__PRIVATE_NEXTJS_INTERNALS_TREE,
})
})
},
[dispatch]
)
}

// Register popstate event to call onPopstate.
useEffect(() => {
// Register popstate event to call onPopstate.
window.addEventListener('popstate', onPopState)
return () => {
if (originalPushState) {
window.history.pushState = originalPushState
}
if (originalReplaceState) {
window.history.replaceState = originalReplaceState
}
window.removeEventListener('popstate', onPopState)
}
}, [onPopState])
}, [dispatch])

const { cache, tree, nextUrl, focusAndScrollRef } =
useUnwrapState(reducerState)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ describe('createInitialRouterState', () => {
tree: initialTree,
canonicalUrl: initialCanonicalUrl,
prefetchCache: new Map(),
pushRef: { pendingPush: false, mpaNavigation: false },
pushRef: {
pendingPush: false,
mpaNavigation: false,
preserveCustomHistoryState: true,
},
focusAndScrollRef: {
apply: false,
onlyHashChange: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@ export function createInitialRouterState({
tree: initialTree,
cache,
prefetchCache: new Map(),
pushRef: { pendingPush: false, mpaNavigation: false },
pushRef: {
pendingPush: false,
mpaNavigation: false,
// First render needs to preserve the previous window.history.state
// to avoid it being overwritten on navigation back/forward with MPA Navigation.
preserveCustomHistoryState: true,
},
focusAndScrollRef: {
apply: false,
onlyHashChange: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import type {
ReducerState,
} from './router-reducer-types'

function isNotUndefined<T>(value: T): value is Exclude<T, undefined> {
return typeof value !== 'undefined'
}

export function handleMutable(
state: ReadonlyReducerState,
mutable: Mutable
Expand All @@ -15,26 +19,28 @@ export function handleMutable(
return {
buildId: state.buildId,
// Set href.
canonicalUrl:
mutable.canonicalUrl != null
? mutable.canonicalUrl === state.canonicalUrl
? state.canonicalUrl
: mutable.canonicalUrl
: state.canonicalUrl,
canonicalUrl: isNotUndefined(mutable.canonicalUrl)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why don't write all of the logic in that file as

canonicalUrl: mutable?.canoniclaUrl ?? state.canonicalUrl,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because it's downleveled in a way that is very code-size intensive

Copy link
Copy Markdown
Contributor

@acdlite acdlite Nov 14, 2023

Choose a reason for hiding this comment

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

In React we just write val !== undefined. (Don’t need to do the typeof thing in strict mode). It’s faster than calling a helper function because you don’t have an extra stack frame.

A function might compress better in some cases (although not always) but the way to think code size is that it’s mostly a heuristic for how long it takes for the VM to parse and compile (that’s why byte for byte JS is way more expensive than, say, an image); in this case the inlined form is very simple.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What I was referring to is that optional chaining and ?? are downleveled, which causes them to explode in size as there are intermediate variables created. In this case we can probably just use mutable.canonicalUrl !== undefined though?

? mutable.canonicalUrl === state.canonicalUrl
? state.canonicalUrl
: mutable.canonicalUrl
: state.canonicalUrl,
pushRef: {
pendingPush:
mutable.pendingPush != null
? mutable.pendingPush
: state.pushRef.pendingPush,
mpaNavigation:
mutable.mpaNavigation != null
? mutable.mpaNavigation
: state.pushRef.mpaNavigation,
pendingPush: isNotUndefined(mutable.pendingPush)
? mutable.pendingPush
: state.pushRef.pendingPush,
mpaNavigation: isNotUndefined(mutable.mpaNavigation)
? mutable.mpaNavigation
: state.pushRef.mpaNavigation,
preserveCustomHistoryState: isNotUndefined(
mutable.preserveCustomHistoryState
)
? mutable.preserveCustomHistoryState
: state.pushRef.preserveCustomHistoryState,
},
// All navigation requires scroll and focus management to trigger.
focusAndScrollRef: {
apply: shouldScroll
? mutable?.scrollableSegments !== undefined
? isNotUndefined(mutable?.scrollableSegments)
? true
: state.focusAndScrollRef.apply
: // If shouldScroll is false then we should not apply scroll and focus management.
Expand Down Expand Up @@ -63,11 +69,12 @@ export function handleMutable(
? mutable.prefetchCache
: state.prefetchCache,
// Apply patched router state.
tree: mutable.patchedTree !== undefined ? mutable.patchedTree : state.tree,
nextUrl:
mutable.patchedTree !== undefined
? computeChangedPath(state.tree, mutable.patchedTree) ??
state.canonicalUrl
: state.nextUrl,
tree: isNotUndefined(mutable.patchedTree)
? mutable.patchedTree
: state.tree,
nextUrl: isNotUndefined(mutable.patchedTree)
? computeChangedPath(state.tree, mutable.patchedTree) ??
state.canonicalUrl
: state.nextUrl,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ function fastRefreshReducerImpl(
return handleMutable(state, mutable)
}

mutable.preserveCustomHistoryState = false

if (!cache.data) {
// TODO-APP: verify that `href` is not an external url.
// Fetch data from the root of the tree.
Expand Down
Loading