fix: export ServerInsertedHTMLContext from next/navigation shim (#145)#151
Conversation
|
@elithrar Approve the workflow. |
|
/bigbonk review please |
commit: |
There was a problem hiding this comment.
Review: ServerInsertedHTMLContext export from next/navigation
The goal of this PR is valid — Apollo Client and other libraries need ServerInsertedHTMLContext exported from next/navigation. However, the current implementation has a fundamental problem: the exported context is disconnected from the actual useServerInsertedHTML mechanism, so libraries like Apollo that call useContext(ServerInsertedHTMLContext) will get null and fail.
How Next.js works
In Next.js, ServerInsertedHTMLContext holds a callback registration function as its value (type: (callback: () => React.ReactNode) => void). The App Router wraps the component tree with <ServerInsertedHTMLContext.Provider value={addCallback}>. When Apollo calls useContext(ServerInsertedHTMLContext), it gets back that addCallback function and uses it to register HTML injection callbacks. Apollo does not call useServerInsertedHTML() — it reads the raw context.
What this PR does
The PR creates a brand new React.createContext(null) and exports it, but:
- Nothing ever wraps the tree with
<ServerInsertedHTMLContext.Provider value={...}> - The context's default value is
null useServerInsertedHTML()doesn't use this context at all — it pushes to a module-level array
So useContext(ServerInsertedHTMLContext) will always return null, which means Apollo will throw:
"The SSR build of ApolloNextAppProvider cannot be used outside of the Next App Router!"
What needs to happen
For this to actually work with Apollo, the ServerInsertedHTMLContext needs to be wired into the rendering pipeline:
- The context should be a singleton (not created separately from
getServerInsertedHTMLContext()— right now there are two independent context objects) - The SSR entry (
app-dev-server.ts) needs to wrap the rendered tree with<ServerInsertedHTMLContext.Provider value={addCallback}>whereaddCallbackis the function that registers callbacks (whatuseServerInsertedHTMLcurrently does internally) useServerInsertedHTMLshould ideally be reimplemented to useuseContext(ServerInsertedHTMLContext)internally, matching Next.js's architecture
This is a more involved change than just adding an export — it requires plumbing the context through the SSR render tree.
Test concerns
The tests verify structural properties of the React Context object ($$typeof, Provider, _currentValue) but never test the actual Apollo integration pattern — calling useContext(ServerInsertedHTMLContext) inside a rendered component and verifying it returns a usable callback function. The "Apollo provider" test (line 58-78) only checks that a function component can be defined, not that it renders correctly or that the context has a value.
| export const ServerInsertedHTMLContext = (() => { | ||
| // Initialize at module load time | ||
| if (typeof React.createContext === "function") { | ||
| return React.createContext<any>(null); | ||
| } | ||
| // In RSC, createContext is not available, return a placeholder | ||
| return null as any; | ||
| })(); |
There was a problem hiding this comment.
Critical: This creates a completely separate React.Context from the one managed by getServerInsertedHTMLContext() (which uses _ServerInsertedHTMLCtx). Two independent contexts means providers/consumers won't share state.
More importantly, this context is created with null as the default value, and nothing in the codebase ever renders a <ServerInsertedHTMLContext.Provider value={...}>. So useContext(ServerInsertedHTMLContext) will always return null.
In Next.js, ServerInsertedHTMLContext holds a callback registration function (type (cb: () => React.ReactNode) => void). The App Router wraps the tree with a Provider whose value is that function. Apollo calls useContext(ServerInsertedHTMLContext) to get it. With this implementation, Apollo's check if (!insertHtml) will be true and it will throw.
To fix this properly, the ServerInsertedHTMLContext needs to be:
- A single shared context (not a second independent one alongside
_ServerInsertedHTMLCtx) - Wired into the SSR render pipeline —
app-dev-server.tsneeds to wrap the tree with<ServerInsertedHTMLContext.Provider value={addCallbackFn}> - Ideally,
useServerInsertedHTMLshould be reimplemented to read from this context viauseContext, matching how Next.js does it
| let _ServerInsertedHTMLCtx: React.Context<any> | null = null; | ||
|
|
||
| /** | ||
| * Get or create the ServerInsertedHTML context. | ||
| * Used internally by libraries that need to hook into the SSR injection system. | ||
| * Returns null in the RSC environment (createContext unavailable). | ||
| * | ||
| * This is exported for compatibility with Apollo Client (@apollo/client-integration-nextjs) | ||
| * and other libraries that may import this directly from next/navigation. | ||
| */ | ||
| export function getServerInsertedHTMLContext(): React.Context<any> | null { | ||
| if (_ServerInsertedHTMLCtx === null && typeof React.createContext === "function") { | ||
| _ServerInsertedHTMLCtx = React.createContext<any>(null); | ||
| } | ||
| return _ServerInsertedHTMLCtx; | ||
| } |
There was a problem hiding this comment.
Dead code: getServerInsertedHTMLContext() manages _ServerInsertedHTMLCtx, but nothing in the codebase calls this function. Meanwhile, the ServerInsertedHTMLContext export below creates its own separate context via the IIFE. These are two independent, disconnected context objects.
If you keep getServerInsertedHTMLContext(), it should return the same context object as ServerInsertedHTMLContext. Or better yet, consolidate into a single approach.
| it("should support Apollo's pattern of wrapping children with ServerInsertedHTMLContext.Provider", async () => { | ||
| const React = await import("react"); | ||
| const mod = await import("../packages/vinext/src/shims/navigation.js"); | ||
| const { ServerInsertedHTMLContext, useServerInsertedHTML } = mod; | ||
|
|
||
| // Simulate how Apollo Client creates a provider component | ||
| const ApolloProvider = ({ children }: { children: React.ReactNode }) => { | ||
| useServerInsertedHTML(() => { | ||
| return "<script>console.log('Apollo SSR');</script>"; | ||
| }); | ||
|
|
||
| return React.createElement( | ||
| ServerInsertedHTMLContext?.Provider, | ||
| { value: null }, | ||
| children | ||
| ); | ||
| }; | ||
|
|
||
| // Verify provider can be created without errors | ||
| expect(ApolloProvider).toBeDefined(); | ||
| expect(typeof ApolloProvider).toBe("function"); |
There was a problem hiding this comment.
This test doesn't actually verify the Apollo integration works. It defines ApolloProvider as a function and checks typeof ApolloProvider === 'function', which is trivially true — it never renders the component or verifies that useContext(ServerInsertedHTMLContext) returns a usable value.
Apollo's actual pattern is:
const insertHtml = useContext(ServerInsertedHTMLContext);
if (!insertHtml) throw new Error(...);A meaningful test would render a component that calls useContext(ServerInsertedHTMLContext) inside a provider tree and verify the context value is a function (not null). That test would currently fail, which is the core issue.
| it("should have ServerInsertedHTMLContext as a React.Context", async () => { | ||
| const mod = await import("../packages/vinext/src/shims/navigation.js"); | ||
| const { ServerInsertedHTMLContext } = mod; | ||
|
|
||
| // React.Context has $$typeof symbol | ||
| expect(ServerInsertedHTMLContext).toHaveProperty("$$typeof"); | ||
| expect(ServerInsertedHTMLContext).toHaveProperty("_currentValue"); | ||
| expect(ServerInsertedHTMLContext).toHaveProperty("Provider"); | ||
| expect(ServerInsertedHTMLContext).toHaveProperty("Consumer"); |
There was a problem hiding this comment.
Testing internal React implementation details (_currentValue, $$typeof) is brittle — these are React internals, not public API. The tests should verify behavior (can components use the context to register callbacks?) rather than structural properties of the context object.
|
The review has been submitted. Here's a summary of the findings: Review Summary for PR #151Requested changes — the PR has the right intent but a fundamental implementation gap. Core IssueThe exported Specific Problems
|
…dflare#145) Wire ServerInsertedHTMLContext into the SSR pipeline so libraries like Apollo Client, styled-components, and emotion that use useContext(ServerInsertedHTMLContext) get a working callback registration function during SSR. Changes: - Add ServerInsertedHTMLContext export to navigation.ts (single shared context, RSC-safe with null fallback) - Wrap SSR rendered tree with ServerInsertedHTMLContext.Provider in handleSsr() — the provider value is useServerInsertedHTML itself, so both direct callers and useContext() callers push to the same ALS-backed callback array - Add TypeScript declaration in next-shims.d.ts - Add 8 integration tests verifying the actual Apollo Client pattern (useContext returns a usable callback, callbacks are flushed correctly) Addresses review feedback from cloudflare#151: - Single context (no dead getServerInsertedHTMLContext) - Provider wired into SSR pipeline (not disconnected) - Tests verify real integration behavior, not React internals
bd564f2 to
8744c2e
Compare
…dflare#145) Wire ServerInsertedHTMLContext into the SSR pipeline so libraries like Apollo Client, styled-components, and emotion that use useContext(ServerInsertedHTMLContext) get a working callback registration function during SSR. Changes: - Add ServerInsertedHTMLContext export to navigation.ts (single shared context, RSC-safe with null fallback) - Wrap SSR rendered tree with ServerInsertedHTMLContext.Provider in handleSsr() — the provider value is useServerInsertedHTML itself, so both direct callers and useContext() callers push to the same ALS-backed callback array - Add TypeScript declaration in next-shims.d.ts - Add 8 integration tests verifying the actual Apollo Client pattern (useContext returns a usable callback, callbacks are flushed correctly) Addresses review feedback from cloudflare#151: - Single context (no dead getServerInsertedHTMLContext) - Provider wired into SSR pipeline (not disconnected) - Tests verify real integration behavior, not React internals
8744c2e to
fde0985
Compare
…dflare#145) Wire ServerInsertedHTMLContext into the SSR pipeline so libraries like Apollo Client, styled-components, and emotion that use useContext(ServerInsertedHTMLContext) get a working callback registration function during SSR. Changes: - Add ServerInsertedHTMLContext export to navigation.ts (single shared context, RSC-safe with null fallback) - Wrap SSR rendered tree with ServerInsertedHTMLContext.Provider in handleSsr() — the provider value is useServerInsertedHTML itself, so both direct callers and useContext() callers push to the same ALS-backed callback array - Add TypeScript declaration in next-shims.d.ts - Add 8 integration tests verifying the actual Apollo Client pattern (useContext returns a usable callback, callbacks are flushed correctly) Addresses review feedback from cloudflare#151: - Single context (no dead getServerInsertedHTMLContext) - Provider wired into SSR pipeline (not disconnected) - Tests verify real integration behavior, not React internals
fde0985 to
4e2ae23
Compare
|
@southpolesteve @elithrar Kindly review the code. |
All review feedback has been addressed in commit 4e2ae23: single shared context (removed dead getServerInsertedHTMLContext), Provider wired into SSR pipeline in handleSsr(), and tests verify real integration behavior with renderToString. CI is fully green.
Issue
Fixes #145: @apollo/client-integration-nextjs requires
ServerInsertedHTMLContextto be exported fromnext/navigationfor proper SSR support.Root Cause Analysis
Apollo Client, styled-components, emotion, StyleX and other CSS-in-JS libraries need to import
ServerInsertedHTMLContextfromnext/navigationfor SSR integration. The context was missing from vinext'snext/navigationshim, causing import errors.This is not a patch—it's a root cause fix addressing a gap in the API surface. vinext aims to provide full Next.js API compatibility, but was missing this documented public export.
Changes
Added to
packages/vinext/src/shims/navigation.ts:ServerInsertedHTMLContextexport - A React Context for CSS-in-JS libraries to inject HTML during SSRgetServerInsertedHTMLContext()helper - Consistent with existinggetLayoutSegmentContext()patternReact.createContextis available (not in react-server condition)Testing
Security
Verification
Type of Change