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.

Sign up for GitHub

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

Ensure consistent component tree for useId #9805

Merged
merged 2 commits into from
Jan 4, 2023
+56 −51
Merged

Ensure consistent component tree for useId #9805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cb0fd9
Ensure consistent component tree for useId
brophdawg11 Jan 3, 2023
190c776
Fix typo
brophdawg11 Jan 4, 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
6 changes: 6 additions & 0 deletions .changeset/three-ladybugs-clap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"react-router": patch
"react-router-dom": patch
---

Ensure useId consistency during SSR
1 change: 0 additions & 1 deletion packages/react-router-dom/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ export {
export {
UNSAFE_DataRouterContext,
UNSAFE_DataRouterStateContext,
UNSAFE_DataStaticRouterContext,
UNSAFE_NavigationContext,
UNSAFE_LocationContext,
UNSAFE_RouteContext,
Expand Down
28 changes: 12 additions & 16 deletions packages/react-router-dom/server.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
Router,
UNSAFE_DataRouterContext as DataRouterContext,
UNSAFE_DataRouterStateContext as DataRouterStateContext,
UNSAFE_DataStaticRouterContext as DataStaticRouterContext,
UNSAFE_enhanceManualRouteObjects as enhanceManualRouteObjects,
} from "react-router-dom";

Expand Down Expand Up @@ -98,6 +97,7 @@ export function StaticRouterProvider({
router,
navigator: getStatelessNavigator(),
static: true,
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of the static prop in favor of the existence of the static context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite yet - we still need static for non-data-routers doing SSR

staticContext: context,
basename: context.basename || "/",
};

Expand All @@ -119,22 +119,18 @@ export function StaticRouterProvider({

return (
<>
<DataStaticRouterContext.Provider value={context}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This outer <DataStaticRouterContext> is removed in favor of a staticContext field on dataRouterContext. Easiest to see with whitespace hidden 😉

<DataRouterContext.Provider value={dataRouterContext}>
<DataRouterStateContext.Provider
value={dataRouterContext.router.state}
<DataRouterContext.Provider value={dataRouterContext}>
<DataRouterStateContext.Provider value={dataRouterContext.router.state}>
<Router
basename={dataRouterContext.basename}
location={dataRouterContext.router.state.location}
navigationType={dataRouterContext.router.state.historyAction}
navigator={dataRouterContext.navigator}
>
<Router
basename={dataRouterContext.basename}
location={dataRouterContext.router.state.location}
navigationType={dataRouterContext.router.state.historyAction}
navigator={dataRouterContext.navigator}
>
<Routes />
</Router>
</DataRouterStateContext.Provider>
</DataRouterContext.Provider>
</DataStaticRouterContext.Provider>
<Routes />
</Router>
</DataRouterStateContext.Provider>
</DataRouterContext.Provider>
{hydrateScript ? (
<script
suppressHydrationWarning
Expand Down
1 change: 0 additions & 1 deletion packages/react-router-native/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ export {
export {
UNSAFE_DataRouterContext,
UNSAFE_DataRouterStateContext,
UNSAFE_DataStaticRouterContext,
UNSAFE_NavigationContext,
UNSAFE_LocationContext,
UNSAFE_RouteContext,
Expand Down
2 changes: 0 additions & 2 deletions packages/react-router/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ import type {
import {
DataRouterContext,
DataRouterStateContext,
DataStaticRouterContext,
LocationContext,
NavigationContext,
RouteContext,
Expand Down Expand Up @@ -239,6 +238,5 @@ export {
RouteContext as UNSAFE_RouteContext,
DataRouterContext as UNSAFE_DataRouterContext,
DataRouterStateContext as UNSAFE_DataRouterStateContext,
DataStaticRouterContext as UNSAFE_DataStaticRouterContext,
enhanceManualRouteObjects as UNSAFE_enhanceManualRouteObjects,
};
49 changes: 29 additions & 20 deletions packages/react-router/lib/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,27 +87,36 @@ export function RouterProvider({

let basename = router.basename || "/";

// The fragment and {null} here are important! We need them to keep React 18's
// useId happy when we are server-rendering since we may have a <script> here
// containing the hydrated server-side staticContext (from StaticRouterProvider).
// useId relies on the component tree structure to generate deterministic id's
// so we need to ensure it remains the same on the client even though
// we don't need the <script> tag
return (
<DataRouterContext.Provider
value={{
router,
navigator,
static: false,
// Do we need this?
basename,
}}
>
<DataRouterStateContext.Provider value={state}>
<Router
basename={router.basename}
location={router.state.location}
navigationType={router.state.historyAction}
navigator={navigator}
>
{router.state.initialized ? <Routes /> : fallbackElement}
</Router>
</DataRouterStateContext.Provider>
</DataRouterContext.Provider>
<>
<DataRouterContext.Provider
value={{
router,
navigator,
static: false,
// Do we need this?
basename,
}}
>
<DataRouterStateContext.Provider value={state}>
<Router
basename={router.basename}
location={router.state.location}
navigationType={router.state.historyAction}
navigator={navigator}
>
{router.state.initialized ? <Routes /> : fallbackElement}
</Router>
</DataRouterStateContext.Provider>
</DataRouterContext.Provider>
{null}
</>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure the structure generated by RouterProvider matches that generated by StaticRouterProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused why we need the null since the docs indicate that only the parent path matters, and our useId calls would be down inside <Routes/>. But without the null I constantly ran into hydration mismatches 😕

Screenshot 2023-01-03 at 4 23 01 PM

Copy link
Member

Choose a reason for hiding this comment

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

Mind filing a question / bug over on the react repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems expected - it might just be a docs issue: facebook/react#22733. Added my own comment in there suggesting a docs update

);
}

Expand Down
8 changes: 1 addition & 7 deletions packages/react-router/lib/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,9 @@ export interface RouteMatch<

export interface DataRouteMatch extends RouteMatch<string, DataRouteObject> {}

// Contexts for data routers
export const DataStaticRouterContext =
React.createContext<StaticHandlerContext | null>(null);
if (__DEV__) {
DataStaticRouterContext.displayName = "DataStaticRouterContext";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an UNSAFE export so ok to remove and add onto DataRouterContext (which is also exported as UNSAFE)

export interface DataRouterContextObject extends NavigationContextObject {
router: Router;
staticContext?: StaticHandlerContext;
}

export const DataRouterContext =
Expand Down
12 changes: 8 additions & 4 deletions packages/react-router/lib/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import {
RouteContext,
RouteErrorContext,
AwaitContext,
DataStaticRouterContext,
} from "./context";

/**
Expand Down Expand Up @@ -564,12 +563,17 @@ interface RenderedRouteProps {
}

function RenderedRoute({ routeContext, match, children }: RenderedRouteProps) {
let dataStaticRouterContext = React.useContext(DataStaticRouterContext);
let dataRouterContext = React.useContext(DataRouterContext);

// Track how deep we got in our render pass to emulate SSR componentDidCatch
// in a DataStaticRouter
if (dataStaticRouterContext && match.route.errorElement) {
dataStaticRouterContext._deepestRenderedBoundaryId = match.route.id;
if (
dataRouterContext &&
dataRouterContext.static &&
dataRouterContext.staticContext &&
match.route.errorElement
) {
dataRouterContext.staticContext._deepestRenderedBoundaryId = match.route.id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only reason we need staticContext around in the component tree - so we can track render depth to bubble SSR render errors

}

return (
Expand Down