Skip to content

Commit

Permalink
Replace custom Thenable type with native Promises (vercel#58337)
Browse files Browse the repository at this point in the history 
Next.js's implementation includes a custom Thenable type based on a
similar one used in React's codebase. It was used to implement a
userspace equivalent of the React.use API before that API became stable,
by throwing a promise-like object and tracking the status on an expando
field. However, it didn't cover all the same cases and behaviors that
React.use does, which led to some subtle bugs like the one fixed by
@ztanner in vercel#55690.

Now that React.use is stable, and we use that for suspending instead of
throwing a promise, we no longer need our custom Thenable type. I've
removed the type and associated functions, and updated our types to use
Promise instead.

Even in cases where a function does return a thenable-object rather than
a native promise, like React Flight's createFromFetch, we should use
TypeScript's built-in PromiseLike utility. Currently, though, we always
await these objects anyway (in fetch-server-response.ts), which turns
them into promises. So Promise is almost always sufficient.

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
  • Loading branch information
@acdlite @kodiakhq
acdlite and kodiakhq[bot] authored Nov 12, 2023
1 parent 00f5b5e commit b425b40
Show file tree
Hide file tree
Showing 16 changed files with 70 additions and 328 deletions.
13 changes: 5 additions & 8 deletions packages/next/src/client/components/layout-router.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { RedirectBoundary } from './redirect-boundary'
import { NotFoundBoundary } from './not-found-boundary'
import { getSegmentValue } from './router-reducer/reducers/get-segment-value'
import { createRouterCacheKey } from './router-reducer/create-router-cache-key'
import { createRecordFromThenable } from './router-reducer/create-record-from-thenable'

/**
* Add refetch marker to router state at the point of the current layout segment.
Expand Down Expand Up @@ -379,13 +378,11 @@ function InnerLayoutRouter({

childNode = {
status: CacheStates.DATA_FETCH,
data: createRecordFromThenable(
fetchServerResponse(
new URL(url, location.origin),
refetchTree,
context.nextUrl,
buildId
)
data: fetchServerResponse(
new URL(url, location.origin),
refetchTree,
context.nextUrl,
buildId
),
subTreeData: null,
head:
Expand Down
6 changes: 2 additions & 4 deletions packages/next/src/client/components/promise-queue.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import type { ThenableRecord } from './router-reducer/router-reducer-types'

/*
This is a simple promise queue that allows you to limit the number of concurrent promises
that are running at any given time. It's used to limit the number of concurrent
Expand All @@ -10,7 +8,7 @@ export class PromiseQueue {
#maxConcurrency: number
#runningCount: number
#queue: Array<{
promiseFn: ThenableRecord<any> | Promise<any>
promiseFn: Promise<any>
task: () => void
}>

Expand Down Expand Up @@ -50,7 +48,7 @@ export class PromiseQueue {
return taskPromise
}

bump(promiseFn: ThenableRecord<any> | Promise<any>) {
bump(promiseFn: Promise<any>) {
const index = this.#queue.findIndex((item) => item.promiseFn === promiseFn)

if (index > -1) {
Expand Down
29 changes: 0 additions & 29 deletions packages/next/src/client/components/router-reducer/create-record-from-thenable.test.ts
View file

This file was deleted.

27 changes: 0 additions & 27 deletions packages/next/src/client/components/router-reducer/create-record-from-thenable.ts
View file

This file was deleted.

19 changes: 7 additions & 12 deletions packages/next/src/client/components/router-reducer/fill-cache-with-data-property.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,16 @@ import type { FetchServerResponseResult } from './fetch-server-response'
import { fillCacheWithDataProperty } from './fill-cache-with-data-property'
import { CacheStates } from '../../../shared/lib/app-router-context.shared-runtime'
import type { CacheNode } from '../../../shared/lib/app-router-context.shared-runtime'
import { createRecordFromThenable } from './create-record-from-thenable'
import type { ThenableRecord } from './router-reducer-types'

describe('fillCacheWithDataProperty', () => {
it('should add data property', () => {
const fetchServerResponseMock: jest.Mock<
ThenableRecord<FetchServerResponseResult>
Promise<FetchServerResponseResult>
> = jest.fn(() =>
createRecordFromThenable(
Promise.resolve([
/* TODO-APP: replace with actual FlightData */ '',
undefined,
])
)
Promise.resolve([
/* TODO-APP: replace with actual FlightData */ '',
undefined,
])
)
const pathname = '/dashboard/settings'
const segments = pathname.split('/')
Expand Down Expand Up @@ -97,9 +94,7 @@ describe('fillCacheWithDataProperty', () => {
</React.Fragment>,
},
"dashboard" => {
"data": Promise {
"status": "pending",
},
"data": Promise {},
"parallelRoutes": Map {},
"status": "DATAFETCH",
"subTreeData": null,
Expand Down
3 changes: 1 addition & 2 deletions packages/next/src/client/components/router-reducer/fill-cache-with-data-property.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { FetchServerResponseResult } from './fetch-server-response'
import type { ThenableRecord } from './router-reducer-types'
import type { FlightSegmentPath } from '../../../server/app-render/types'
import { CacheStates } from '../../../shared/lib/app-router-context.shared-runtime'
import type { CacheNode } from '../../../shared/lib/app-router-context.shared-runtime'
Expand All @@ -12,7 +11,7 @@ export function fillCacheWithDataProperty(
newCache: CacheNode,
existingCache: CacheNode,
flightSegmentPath: FlightSegmentPath,
fetchResponse: () => ThenableRecord<FetchServerResponseResult>,
fetchResponse: () => Promise<FetchServerResponseResult>,
bailOnParallelRoutes: boolean = false
): { bailOptimistic: boolean } | undefined {
const isLastEntry = flightSegmentPath.length <= 2
Expand Down
13 changes: 5 additions & 8 deletions packages/next/src/client/components/router-reducer/reducers/fast-refresh-reducer.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { fetchServerResponse } from '../fetch-server-response'
import { createRecordFromThenable } from '../create-record-from-thenable'
import { createHrefFromUrl } from '../create-href-from-url'
import { applyRouterStatePatchToTree } from '../apply-router-state-patch-to-tree'
import { isNavigatingToNewRootLayout } from '../is-navigating-to-new-root-layout'
Expand Down Expand Up @@ -30,13 +29,11 @@ function fastRefreshReducerImpl(
if (!cache.data) {
// TODO-APP: verify that `href` is not an external url.
// Fetch data from the root of the tree.
cache.data = createRecordFromThenable(
fetchServerResponse(
new URL(href, origin),
[state.tree[0], state.tree[1], state.tree[2], 'refetch'],
state.nextUrl,
state.buildId
)
cache.data = fetchServerResponse(
new URL(href, origin),
[state.tree[0], state.tree[1], state.tree[2], 'refetch'],
state.nextUrl,
state.buildId
)
}

Expand Down
128 changes: 4 additions & 124 deletions packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,37 +274,7 @@ describe('navigateReducer', () => {
"nextUrl": "/linking/about",
"prefetchCache": Map {
"/linking/about" => {
"data": Promise {
"status": "fulfilled",
"value": [
[
[
"children",
"linking",
"children",
"about",
[
"about",
{
"children": [
"__PAGE__",
{},
],
},
],
<h1>
About Page!
</h1>,
<React.Fragment>
<title>
About page!
</title>
</React.Fragment>,
],
],
undefined,
],
},
"data": Promise {},
"kind": "temporary",
"lastUsedTime": 1690329600000,
"prefetchTime": 1690329600000,
Expand Down Expand Up @@ -973,37 +943,7 @@ describe('navigateReducer', () => {
</React.Fragment>,
},
"about" => {
"data": Promise {
"status": "fulfilled",
"value": [
[
[
"children",
"linking",
"children",
"about",
[
"about",
{
"children": [
"__PAGE__",
{},
],
},
],
<h1>
About Page!
</h1>,
<React.Fragment>
<title>
About page!
</title>
</React.Fragment>,
],
],
undefined,
],
},
"data": Promise {},
"parallelRoutes": Map {},
"status": "DATAFETCH",
"subTreeData": null,
Expand Down Expand Up @@ -1422,37 +1362,7 @@ describe('navigateReducer', () => {
"nextUrl": "/linking/about",
"prefetchCache": Map {
"/linking/about" => {
"data": Promise {
"status": "fulfilled",
"value": [
[
[
"children",
"linking",
"children",
"about",
[
"about",
{
"children": [
"__PAGE__",
{},
],
},
],
<h1>
About Page!
</h1>,
<React.Fragment>
<title>
About page!
</title>
</React.Fragment>,
],
],
undefined,
],
},
"data": Promise {},
"kind": "auto",
"lastUsedTime": null,
"prefetchTime": 1690329600000,
Expand Down Expand Up @@ -2063,37 +1973,7 @@ describe('navigateReducer', () => {
"nextUrl": "/linking/about",
"prefetchCache": Map {
"/linking/about" => {
"data": Promise {
"status": "fulfilled",
"value": [
[
[
"children",
"linking",
"children",
"about",
[
"about",
{
"children": [
"__PAGE__",
{},
],
},
],
<h1>
About Page!
</h1>,
<React.Fragment>
<title>
About page!
</title>
</React.Fragment>,
],
],
undefined,
],
},
"data": Promise {},
"kind": "temporary",
"lastUsedTime": 1690329600000,
"prefetchTime": 1690329600000,
Expand Down
Loading

0 comments on commit b425b40

Please sign in to comment.