Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suspend in render, not in reducers #56497

Merged
merged 42 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
95f6575
(wip) don't suspend in reducers
ztanner Oct 5, 2023
7caa298
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 5, 2023
e4b63d6
rm log
ztanner Oct 5, 2023
2f01913
debug
ztanner Oct 5, 2023
d6e699c
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 5, 2023
245edbc
sequencing logic
ztanner Oct 6, 2023
1cc15fa
test
ztanner Oct 7, 2023
0010b84
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 7, 2023
4a2a9c0
lint imports
ztanner Oct 7, 2023
13f7a01
tweak
ztanner Oct 7, 2023
2376810
test tweak
ztanner Oct 7, 2023
d669590
revert test changes
ztanner Oct 10, 2023
7398933
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 10, 2023
3235239
another test tweak
ztanner Oct 10, 2023
4eea5cf
first pass at a queue
ztanner Oct 11, 2023
9f47f6f
cleanup
ztanner Oct 11, 2023
e4d7c2d
move some things around
ztanner Oct 11, 2023
7182bc5
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 11, 2023
14f808e
lint
ztanner Oct 11, 2023
f5a7bbb
create mutable in hydrate
ztanner Oct 11, 2023
6061836
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 11, 2023
29ae730
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 11, 2023
3091bdc
improve queue appending logic
ztanner Oct 12, 2023
4a847e2
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 13, 2023
4d86fc7
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 13, 2023
a6d573b
call setState immediately when action is dispatched
ztanner Oct 13, 2023
2fbd126
rename for clarity
ztanner Oct 13, 2023
651db2a
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 13, 2023
117d9bf
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 13, 2023
1a59742
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 16, 2023
eb7a6a7
re-arrange some things
ztanner Oct 16, 2023
dcfc92b
Remove ! as the type is inferred correctly
timneutkens Oct 17, 2023
7aab590
remove withResolvers in place of using a promise constructor
ztanner Oct 17, 2023
8e0c0d1
re-add isThenable typeguard
ztanner Oct 17, 2023
2b145c1
add action discarding for navigation events
ztanner Oct 17, 2023
ec8a37e
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 17, 2023
2966f85
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 20, 2023
00660cf
refresh + test
ztanner Oct 20, 2023
d3f199d
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 20, 2023
94f6bb2
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 21, 2023
de848d0
Merge branch 'canary' into feat/remove-suspending-reducers
ztanner Oct 31, 2023
a41dbd7
pr comments
ztanner Oct 31, 2023
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
14 changes: 11 additions & 3 deletions packages/next/src/client/app-index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import { GlobalLayoutRouterContext } from '../shared/lib/app-router-context.shar
import onRecoverableError from './on-recoverable-error'
import { callServer } from './app-call-server'
import { isNextRouterError } from './components/is-next-router-error'
import {
ActionQueueContext,
createMutableActionQueue,
} from '../shared/lib/router/action-queue'

// Since React doesn't call onerror for errors caught in error boundaries.
const origConsoleError = window.console.error
Expand Down Expand Up @@ -222,16 +226,20 @@ export function hydrate() {
}
}

const actionQueue = createMutableActionQueue()

const reactEl = (
<StrictModeIfEnabled>
<HeadManagerContext.Provider
value={{
appDir: true,
}}
>
<Root>
<RSCComponent />
</Root>
<ActionQueueContext.Provider value={actionQueue}>
<Root>
<RSCComponent />
</Root>
</ActionQueueContext.Provider>
</HeadManagerContext.Provider>
</StrictModeIfEnabled>
)
Expand Down
55 changes: 24 additions & 31 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import type {
FlightData,
} from '../../server/app-render/types'
import type { ErrorComponent } from './error-boundary'
import { reducer } from './router-reducer/router-reducer'
import {
ACTION_FAST_REFRESH,
ACTION_NAVIGATE,
Expand All @@ -36,7 +35,6 @@ import {
PrefetchKind,
} from './router-reducer/router-reducer-types'
import type {
Mutable,
ReducerActions,
RouterChangeByServerResponse,
RouterNavigate,
Expand All @@ -47,7 +45,10 @@ import {
SearchParamsContext,
PathnameContext,
} from '../../shared/lib/hooks-client-context.shared-runtime'
import { useReducerWithReduxDevtools } from './use-reducer-with-devtools'
import {
useReducerWithReduxDevtools,
useUnwrapState,
} from './use-reducer-with-devtools'
import { ErrorBoundary } from './error-boundary'
import { createInitialRouterState } from './router-reducer/create-initial-router-state'
import type { InitialRouterStateParameters } from './router-reducer/create-initial-router-state'
Expand All @@ -73,9 +74,9 @@ export function getServerActionDispatcher() {
return globalServerActionDispatcher
}

let globalMutable: Mutable['globalMutable'] = {
refresh: () => {}, // noop until the router is initialized
}
const globalMutable: {
pendingMpaPath?: string
} = {}

export function urlToUrlWithoutFlightMarker(url: string): URL {
const urlWithoutFlightParameters = new URL(url, location.origin)
Expand Down Expand Up @@ -131,7 +132,7 @@ function HistoryUpdater({ tree, pushRef, canonicalUrl, sync }: any) {
return null
}

const createEmptyCacheNode = () => ({
export const createEmptyCacheNode = () => ({
status: CacheStates.LAZY_INITIALIZED,
data: null,
subTreeData: null,
Expand All @@ -145,7 +146,7 @@ function useServerActionDispatcher(dispatch: React.Dispatch<ReducerActions>) {
dispatch({
...actionPayload,
type: ACTION_SERVER_ACTION,
mutable: { globalMutable },
mutable: {},
cache: createEmptyCacheNode(),
})
})
Expand Down Expand Up @@ -174,7 +175,7 @@ function useChangeByServerResponse(
previousTree,
overrideCanonicalUrl,
cache: createEmptyCacheNode(),
mutable: { globalMutable },
mutable: {},
})
})
},
Expand All @@ -186,7 +187,6 @@ function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
return useCallback(
(href, navigateType, forceOptimisticNavigation, shouldScroll) => {
const url = new URL(addBasePath(href), location.href)
globalMutable.pendingNavigatePath = createHrefFromUrl(url)

return dispatch({
type: ACTION_NAVIGATE,
Expand All @@ -197,7 +197,7 @@ function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
shouldScroll: shouldScroll ?? true,
navigateType,
cache: createEmptyCacheNode(),
mutable: { globalMutable },
mutable: {},
})
},
[dispatch]
Expand Down Expand Up @@ -229,25 +229,15 @@ function Router({
}),
[buildId, children, initialCanonicalUrl, initialTree, initialHead]
)
const [
{
tree,
cache,
prefetchCache,
pushRef,
focusAndScrollRef,
canonicalUrl,
nextUrl,
},
dispatch,
sync,
] = useReducerWithReduxDevtools(reducer, initialState)
const [reducerState, dispatch, sync] =
useReducerWithReduxDevtools(initialState)
Comment on lines +232 to +233
Copy link
Contributor

Choose a reason for hiding this comment

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

What's in this initial state that can't be created before render, at the same time that the queue is created?

Copy link
Member Author

@ztanner ztanner Oct 17, 2023

Choose a reason for hiding this comment

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

This relies on data that is streamed in from app-render (such as the initial tree) and I wasn't seeing how we could easily access that data during hydration.

cc @timneutkens in case you have any ideas here, this was the thing we chatted about last week

EDIT: We'll see about doing this in a follow-up, as the changes are a bit more involved and don't necessarily need to block this


useEffect(() => {
// Ensure initialParallelRoutes is cleaned up from memory once it's used.
initialParallelRoutes = null!
}, [])

const { canonicalUrl } = useUnwrapState(reducerState)
// Add memoized pathname/query for useSearchParams and usePathname.
const { searchParams, pathname } = useMemo(() => {
const url = new URL(
Expand Down Expand Up @@ -322,7 +312,7 @@ function Router({
dispatch({
type: ACTION_REFRESH,
cache: createEmptyCacheNode(),
mutable: { globalMutable },
mutable: {},
origin: window.location.origin,
})
})
Expand All @@ -338,7 +328,7 @@ function Router({
dispatch({
type: ACTION_FAST_REFRESH,
cache: createEmptyCacheNode(),
mutable: { globalMutable },
mutable: {},
origin: window.location.origin,
})
})
Expand All @@ -356,11 +346,10 @@ function Router({
}
}, [appRouter])

useEffect(() => {
globalMutable.refresh = appRouter.refresh
}, [appRouter.refresh])

if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks
const { cache, prefetchCache, tree } = useUnwrapState(reducerState)

// This hook is in a conditional but that is ok because `process.env.NODE_ENV` never changes
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
Expand Down Expand Up @@ -408,6 +397,7 @@ function Router({
// probably safe because we know this is a singleton component and it's never
// in <Offscreen>. At least I hope so. (It will run twice in dev strict mode,
// but that's... fine?)
const { pushRef } = useUnwrapState(reducerState)
if (pushRef.mpaNavigation) {
// if there's a re-render, we don't want to trigger another redirect if one is already in flight to the same URL
if (globalMutable.pendingMpaPath !== canonicalUrl) {
Expand Down Expand Up @@ -466,6 +456,9 @@ function Router({
}
}, [onPopState])

const { cache, tree, nextUrl, focusAndScrollRef } =
useUnwrapState(reducerState)

const head = useMemo(() => {
return findHeadInCache(cache, tree[1])
}, [cache, tree])
Expand Down Expand Up @@ -513,7 +506,7 @@ function Router({
<LayoutRouterContext.Provider
value={{
childNodes: cache.parallelRoutes,
tree: tree,
tree,
// Root node always has `url`
// Provided in AppTreeContext to ensure it can be overwritten in layout-router
url: canonicalUrl,
Expand Down

This file was deleted.

This file was deleted.

Loading
Loading