Skip to content

Commit

Permalink
Updates our sync property access for params and searchParams to allow…
Browse files Browse the repository at this point in the history 
… value (#70570)

The reasoning is that React only reads this after having set it. As long
as we allow this to be written to we can set it to the param or
searchParam value initially.
  • Loading branch information
@gnoff
gnoff authored Sep 27, 2024
1 parent e701b37 commit e3fcabe
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 36 deletions.
10 changes: 6 additions & 4 deletions packages/next/src/server/request/params.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,6 @@ function makeAbortingExoticParams(

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -284,7 +283,6 @@ function makeErroringExoticParams(

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -394,7 +392,6 @@ function makeUntrackedExoticParams(underlyingParams: Params): Promise<Params> {

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -450,7 +447,6 @@ function makeDynamicallyTrackedExoticParamsWithDevWarnings(

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -483,6 +479,12 @@ function makeDynamicallyTrackedExoticParamsWithDevWarnings(
}
return ReflectAdapter.get(target, prop, receiver)
},
set(target, prop, value, receiver) {
if (typeof prop === 'string') {
proxiedProperties.delete(prop)
}
return ReflectAdapter.set(target, prop, value, receiver)
},
ownKeys(target) {
warnForEnumeration(store.route, unproxiedProperties)
return Reflect.ownKeys(target)
Expand Down
16 changes: 6 additions & 10 deletions packages/next/src/server/request/search-params.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,6 @@ function makeAbortingExoticSearchParams(
case 'catch':
case 'finally':

// React Promise extension
// fallthrough
case 'value':

// Common tested properties
// fallthrough
case 'toJSON':
Expand Down Expand Up @@ -307,10 +303,6 @@ function makeErroringExoticSearchParams(
case 'catch':
case 'finally':

// React Promise extension
// fallthrough
case 'value':

// Common tested properties
// fallthrough
case 'toJSON':
Expand Down Expand Up @@ -471,7 +463,6 @@ function makeUntrackedExoticSearchParams(

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -593,7 +584,6 @@ function makeDynamicallyTrackedExoticSearchParamsWithDevWarnings(

// React Promise extension
// fallthrough
case 'value':
case 'status':

// Common tested properties
Expand Down Expand Up @@ -641,6 +631,12 @@ function makeDynamicallyTrackedExoticSearchParamsWithDevWarnings(
}
return ReflectAdapter.get(target, prop, receiver)
},
set(target, prop, value, receiver) {
if (typeof prop === 'string') {
proxiedProperties.delete(prop)
}
return Reflect.set(target, prop, value, receiver)
},
has(target, prop) {
if (typeof prop === 'string') {
const expression = describeHasCheckingStringProperty(
Expand Down
36 changes: 22 additions & 14 deletions test/e2e/app-dir/dynamic-io/dynamic-io.params.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2351,7 +2351,7 @@ describe('dynamic-io', () => {
}
})

it('should not allow param names like then, value, and status when accessing params directly in a server component', async () => {
it('should not allow param names like then and status when accessing params directly in a server component', async () => {
expect(getLines('In route /params')).toEqual([])
let $ = await next.render$(
'/params/shadowing/foo/bar/baz/qux/sync/layout/server'
Expand All @@ -2363,14 +2363,16 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([
expect.stringContaining(
'The following properties were not copied: `then`, `value`, , and `status`.'
'The following properties were not copied: `then` and `status`.'
),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
])
} else {
expect($('#layout').text()).toBe('at runtime')
Expand All @@ -2379,7 +2381,7 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([])
}
Expand All @@ -2394,14 +2396,16 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([
expect.stringContaining(
'The following properties were not copied: `then`, `value`, , and `status`.'
'The following properties were not copied: `then` and `status`.'
),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
])
} else {
expect($('#layout').text()).toBe('at runtime')
Expand All @@ -2410,13 +2414,13 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([])
}
})

it('should not allow param names like then, value, and status when accessing params directly in a client component', async () => {
it('should not allow param names like then and status when accessing params directly in a client component', async () => {
expect(getLines('In route /params')).toEqual([])
let $ = await next.render$(
'/params/shadowing/foo/bar/baz/qux/sync/layout/client'
Expand All @@ -2428,14 +2432,16 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([
expect.stringContaining(
'The following properties were not copied: `then`, `value`, , and `status`.'
'The following properties were not copied: `then` and `status`.'
),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
])
} else {
expect($('#layout').text()).toBe('at runtime')
Expand All @@ -2444,7 +2450,7 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([])
}
Expand All @@ -2459,14 +2465,16 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([
expect.stringContaining(
'The following properties were not copied: `then`, `value`, , and `status`.'
'The following properties were not copied: `then` and `status`.'
),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
expect.stringContaining('accessed directly with `params.dyn`'),
expect.stringContaining('accessed directly with `params.value`'),
])
} else {
expect($('#layout').text()).toBe('at runtime')
Expand All @@ -2475,7 +2483,7 @@ describe('dynamic-io', () => {
expect($('#param-then').text()).toEqual(
expect.stringContaining('native code')
)
expect($('#param-value').text()).toBe('undefined')
expect($('#param-value').text()).toBe('baz')
expect($('#param-status').text()).toBe('undefined')
expect(getLines('In route /params')).toEqual([])
}
Expand Down
36 changes: 28 additions & 8 deletions test/e2e/app-dir/dynamic-io/dynamic-io.search.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,10 @@ describe('dynamic-io', () => {
let searchWarnings = getLines('In route /search')
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
expect(searchWarnings).toEqual([
expect.stringContaining(
Expand All @@ -400,6 +401,9 @@ describe('dynamic-io', () => {
'accessed directly with `searchParams.sentinel`'
),
expect.stringContaining('accessed directly with `searchParams.foo`'),
expect.stringContaining(
'accessed directly with `searchParams.value`'
),
])
} else {
expect(searchWarnings).toHaveLength(0)
Expand All @@ -408,9 +412,10 @@ describe('dynamic-io', () => {
// This test case aborts synchronously and the later component render
// triggers the outer boundary
expect($('main').text()).toContain('outer loading...')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
}

Expand Down Expand Up @@ -450,9 +455,10 @@ describe('dynamic-io', () => {
let searchWarnings = getLines('In route /search')
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
expect(searchWarnings).toEqual([
expect.stringContaining(
Expand All @@ -462,15 +468,19 @@ describe('dynamic-io', () => {
'accessed directly with `searchParams.sentinel`'
),
expect.stringContaining('accessed directly with `searchParams.foo`'),
expect.stringContaining(
'accessed directly with `searchParams.value`'
),
])
} else {
expect(searchWarnings).toHaveLength(0)
expect($('#layout').text()).toBe('at buildtime')
expect($('main').text()).toContain('inner loading...')
expect($('main').text()).not.toContain('outer loading...')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
}

Expand Down Expand Up @@ -690,9 +700,10 @@ describe('dynamic-io', () => {
let searchWarnings = getLines('In route /search')
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
expect(searchWarnings).toEqual([
expect.stringContaining(
Expand All @@ -702,13 +713,17 @@ describe('dynamic-io', () => {
'accessed directly with `searchParams.sentinel`'
),
expect.stringContaining('accessed directly with `searchParams.foo`'),
expect.stringContaining(
'accessed directly with `searchParams.value`'
),
])
} else {
expect(searchWarnings).toHaveLength(0)
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
}

Expand Down Expand Up @@ -748,9 +763,10 @@ describe('dynamic-io', () => {
let searchWarnings = getLines('In route /search')
if (isNextDev) {
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
expect(searchWarnings).toEqual([
expect.stringContaining(
Expand All @@ -760,13 +776,17 @@ describe('dynamic-io', () => {
'accessed directly with `searchParams.sentinel`'
),
expect.stringContaining('accessed directly with `searchParams.foo`'),
expect.stringContaining(
'accessed directly with `searchParams.value`'
),
])
} else {
expect(searchWarnings).toHaveLength(0)
expect($('#layout').text()).toBe('at runtime')
expect($('[data-value]').length).toBe(2)
expect($('[data-value]').length).toBe(3)
expect($('#value-sentinel').text()).toBe('hello')
expect($('#value-foo').text()).toBe('foo')
expect($('#value-value').text()).toBe('baz')
expect($('#page').text()).toBe('at runtime')
}

Expand Down

0 comments on commit e3fcabe

Please sign in to comment.