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

Handle special props ref and key in path and search params #5537

Merged
merged 21 commits into from
Sep 4, 2022
Merged
Show file tree
Hide file tree
Changes from 16 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
20 changes: 20 additions & 0 deletions packages/router/src/__tests__/router.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1489,3 +1489,23 @@ test('params should be updated if navigated to different route with same page',
expect(screen.queryByText('hookparams 99')).toBeInTheDocument()
})
})

test('should handle ref and key as search params', async () => {
const ParamsPage = () => {
const { ref, key } = useParams()
return <p>{JSON.stringify({ ref, key })}</p>
}

const TestRouter = () => (
<Router>
<Route path="/params" page={ParamsPage} name="params" />
</Router>
)

const screen = render(<TestRouter />)
act(() => navigate('/params?ref=1&key=2'))

await waitFor(() => {
expect(screen.queryByText(`{"ref":"1","key":"2"}`)).toBeInTheDocument()
})
})
20 changes: 20 additions & 0 deletions packages/router/src/__tests__/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,26 @@ describe('validatePath', () => {
])('validates correct path "%s"', (path) => {
expect(validatePath.bind(null, path)).not.toThrow()
})

it.each(['/path/{ref}', '/path/{ref:Int}', '/path/{key}', '/path/{key:Int}'])(
jtoar marked this conversation as resolved.
Show resolved Hide resolved
'rejects paths with ref or key as path parameters: "%s"',
(path) => {
expect(validatePath.bind(null, path)).toThrowError(
[
`Route contains ref or key as a path parameter: "${path}"`,
"`ref` and `key` shouldn't be used as path parameters because they're special React props.",
'You can fix this by renaming the path parameter.',
].join('\n')
)
}
)

it.each(['/path/{reff}', '/path/{reff:Int}', '/path/{keys}', '/path/{keys:Int}'])(
jtoar marked this conversation as resolved.
Show resolved Hide resolved
`doesn't reject paths with variations on ref or key as path parameters: "%s"`,
(path) => {
expect(validatePath.bind(null, path)).not.toThrowError()
}
)
})

describe('parseSearch', () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/router/src/router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ const InternalRoute = ({
)

const searchParams = parseSearch(location.search)
const allParams = { ...searchParams, ...pathParams }
const allParams: Record<string, string> = { ...searchParams, ...pathParams }

if (redirect) {
const newPath = replaceParams(redirect, allParams)
Expand All @@ -113,6 +113,14 @@ const InternalRoute = ({

const Page = activePageContext.loadingState[path]?.page || (() => null)

// There are two special props in React: `ref` and `key`. (See https://reactjs.org/warnings/special-props.html.)
// It's very possible that the URL has `ref` as a search param (e.g. https://redwoodjs.com/?ref=producthunt).
// Since we pass URL params to the page, we have to be careful not to pass `ref` or `key`, otherwise the page will break.
// (The page won't actually break if `key` is passed, but it feels unclean.)
// If users want to access them, they can use `useParams`.
delete allParams['ref']
delete allParams['key']

// Level 3/3 (InternalRoute)
return <Page {...allParams} />
}
Expand Down
10 changes: 10 additions & 0 deletions packages/router/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,16 @@ export const validatePath = (path: string) => {
throw new Error(`Route path contains spaces: "${path}"`)
}

if (/\{(ref|key)(:|\})/.test(path)) {
jtoar marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(
[
`Route contains ref or key as a path parameter: "${path}"`,
"`ref` and `key` shouldn't be used as path parameters because they're special React props.",
'You can fix this by renaming the path parameter.',
].join('\n')
)
}

// Check for duplicate named params.
const matches = path.matchAll(/\{([^}]+)\}/g)
const memo: Record<string, boolean> = {}
Expand Down