fix(router): warn when Outlet is rendered in fallback components - #8045
Merged
Conversation
Contributor
|
View your CI Pipeline Execution ↗ for commit d429d9c
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview4 package(s) bumped directly, 19 bumped as dependents. 🟩 Patch bumps
|
Contributor
📝 WalkthroughWalkthroughReact, Solid, and Vue Router now wrap pending, error, and not-found components with development-only context. ChangesOutlet context warning
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Sequence Diagram(s)sequenceDiagram
participant Match
participant nonRouteComponentContext
participant Outlet
participant Console
Match->>nonRouteComponentContext: Provide pending, error, or notFound context
Outlet->>nonRouteComponentContext: Read component context
nonRouteComponentContext-->>Outlet: Return component identifier
Outlet->>Console: Log development warning
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-router/tests/Outlet.test.tsx`:
- Around line 49-146: Extend the Outlet warning coverage with router-level
fallback cases: configure defaultPendingComponent, defaultErrorComponent, and
defaultNotFoundComponent in createRouter while omitting the corresponding
route-level options. Mirror the existing pending, error, and not-found scenarios
and assert outletWarning uses each fallback component name.
In `@packages/vue-router/src/nonRouteComponentContext.tsx`:
- Around line 35-45: Update the exported renderInNonRouteComponentContext props
parameter to remove any, using unknown or the appropriate Vue raw-props type
accepted by Vue.h while preserving support for optional props. Keep the existing
component rendering and context-provider behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ef4b981-d6c6-4420-b089-7b3a836604d8
📒 Files selected for processing (18)
.changeset/friendly-outlets-warn.mdpackages/react-router/src/CatchBoundary.tsxpackages/react-router/src/Match.tsxpackages/react-router/src/nonRouteComponentContext.tsxpackages/react-router/src/renderRouteNotFound.tsxpackages/react-router/tests/Outlet.test.tsxpackages/solid-router/src/CatchBoundary.tsxpackages/solid-router/src/Match.tsxpackages/solid-router/src/Matches.tsxpackages/solid-router/src/nonRouteComponentContext.tsxpackages/solid-router/src/renderRouteNotFound.tsxpackages/solid-router/tests/Outlet.test.tsxpackages/vue-router/src/CatchBoundary.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/Matches.tsxpackages/vue-router/src/nonRouteComponentContext.tsxpackages/vue-router/src/renderRouteNotFound.tsxpackages/vue-router/tests/Outlet.test.tsx
Comment on lines
+49
to
+146
| test('warns when Outlet is rendered inside a pendingComponent', async () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
| const pending = createControlledPromise<void>() | ||
| const rootRoute = createRootRoute({ component: Outlet }) | ||
| const indexRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/', | ||
| component: () => <span>Index route</span>, | ||
| }) | ||
| const pendingRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/pending', | ||
| loader: () => pending, | ||
| pendingMs: 0, | ||
| pendingComponent: () => ( | ||
| <> | ||
| <span>Pending route</span> | ||
| <Outlet /> | ||
| </> | ||
| ), | ||
| component: () => <span>Resolved route</span>, | ||
| }) | ||
| const router = createRouter({ | ||
| routeTree: rootRoute.addChildren([indexRoute, pendingRoute]), | ||
| history: createMemoryHistory({ initialEntries: ['/'] }), | ||
| }) | ||
|
|
||
| render(<RouterProvider router={router} />) | ||
| await screen.findByText('Index route') | ||
|
|
||
| const navigation = router.navigate({ to: '/pending' }) | ||
| expect(await screen.findByText('Pending route')).toBeInTheDocument() | ||
| pending.resolve() | ||
| await navigation | ||
|
|
||
| expect(warn).toHaveBeenCalledWith(outletWarning('pendingComponent')) | ||
| }) | ||
|
|
||
| test('warns when Outlet is rendered inside an errorComponent', async () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
| const rootRoute = createRootRoute({ component: Outlet }) | ||
| const indexRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/', | ||
| loader: () => { | ||
| throw new Error('Loader failed') | ||
| }, | ||
| errorComponent: () => ( | ||
| <> | ||
| <span>Error route</span> | ||
| <Outlet /> | ||
| </> | ||
| ), | ||
| }) | ||
| const router = createRouter({ | ||
| routeTree: rootRoute.addChildren([indexRoute]), | ||
| history: createMemoryHistory({ initialEntries: ['/'] }), | ||
| }) | ||
|
|
||
| render(<RouterProvider router={router} />) | ||
|
|
||
| expect(await screen.findByText('Error route')).toBeInTheDocument() | ||
| expect(warn).toHaveBeenCalledWith(outletWarning('errorComponent')) | ||
| }) | ||
|
|
||
| test('warns when Outlet is rendered inside a notFoundComponent', async () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
| const rootRoute = createRootRoute({ component: Outlet }) | ||
| const indexRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/', | ||
| component: () => <span>Index route</span>, | ||
| }) | ||
| const notFoundRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/not-found', | ||
| component: () => { | ||
| throw notFound() | ||
| }, | ||
| notFoundComponent: () => ( | ||
| <> | ||
| <span>Not found route</span> | ||
| <Outlet /> | ||
| </> | ||
| ), | ||
| }) | ||
| const router = createRouter({ | ||
| routeTree: rootRoute.addChildren([indexRoute, notFoundRoute]), | ||
| history: createMemoryHistory({ initialEntries: ['/'] }), | ||
| }) | ||
|
|
||
| render(<RouterProvider router={router} />) | ||
| await screen.findByText('Index route') | ||
| await router.navigate({ to: '/not-found' }) | ||
|
|
||
| expect(await screen.findByText('Not found route')).toBeInTheDocument() | ||
| expect(warn).toHaveBeenCalledWith(outletWarning('notFoundComponent')) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add router-level fallback warning tests.
These tests cover only route-level pendingComponent, errorComponent, and notFoundComponent options. Add cases that configure defaultPendingComponent, defaultErrorComponent, and defaultNotFoundComponent on createRouter while the affected route omits its equivalent option.
This is required by the PR objective for router-level fallback coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-router/tests/Outlet.test.tsx` around lines 49 - 146, Extend
the Outlet warning coverage with router-level fallback cases: configure
defaultPendingComponent, defaultErrorComponent, and defaultNotFoundComponent in
createRouter while omitting the corresponding route-level options. Mirror the
existing pending, error, and not-found scenarios and assert outletWarning uses
each fallback component name.
Comment on lines
+35
to
+45
| export function renderInNonRouteComponentContext( | ||
| component: Vue.Component, | ||
| props: Record<string, any> | undefined, | ||
| context: NonRouteComponent, | ||
| ): Vue.VNode { | ||
| return Vue.h( | ||
| NonRouteComponentContextProvider!, | ||
| { value: context }, | ||
| { default: () => Vue.h(component, props) }, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove any from the exported props contract.
Record<string, any> disables type checking for every prop passed through this helper. Use unknown or a Vue raw-props type that is compatible with Vue.h.
As per coding guidelines, **/*.{ts,tsx} must use “TypeScript strict mode with extensive type safety.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vue-router/src/nonRouteComponentContext.tsx` around lines 35 - 45,
Update the exported renderInNonRouteComponentContext props parameter to remove
any, using unknown or the appropriate Vue raw-props type accepted by Vue.h while
preserving support for optional props. Keep the existing component rendering and
context-provider behavior unchanged.
Source: Coding guidelines
Merging this PR will degrade performance by 13.55%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem server error-paths not-found (react) |
414.1 KB | 398.2 KB | +3.99% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
331.7 KB | 320.2 KB | +3.59% |
| ⚡ | Memory | mem server error-paths error (react) |
1,020.9 KB | 988 KB | +3.33% |
| ⚡ | Memory | mem server server-fn-churn (react) |
355.8 KB | 344.6 KB | +3.23% |
| 👁 | Simulation | ssr request loop (solid) |
434.1 ms | 449.4 ms | -3.41% |
| 👁 | Simulation | ssr server-fn during document ssr (vue) |
668.9 ms | 690.6 ms | -3.14% |
| 👁 | Memory | mem server error-paths redirect (solid) |
379.9 KB | 393.1 KB | -3.35% |
| 👁 | Memory | mem server peak-large-page (solid) |
1.1 MB | 1.2 MB | -8.29% |
| 👁 | Memory | mem server error-paths not-found (vue) |
777.5 KB | 2,370.8 KB | -67.21% |
| 👁 | Memory | mem server error-paths unmatched (react) |
422.5 KB | 702.4 KB | -39.84% |
| 👁 | Memory | mem server request-churn (react) |
679.8 KB | 706.5 KB | -3.78% |
| 👁 | Memory | mem client navigation-churn (vue) |
1.6 MB | 1.7 MB | -3.62% |
| 👁 | Memory | mem client unique-location-churn (vue) |
1.3 MB | 1.3 MB | -3.61% |
| 👁 | Memory | mem client navigation-churn (solid) |
693.2 KB | 914 KB | -24.16% |
| 👁 | Memory | mem client unique-location-churn (solid) |
426.5 KB | 482.4 KB | -11.6% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/outlet-fallback-warning (d429d9c) with main (7e93431)
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8042
Summary
Outletis rendered inside pending, error, or not-found componentsProduction bundles
Tests
CI=1 NX_DAEMON=false pnpm nx affected --target=test:eslint --exclude="examples/**,e2e/**" --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx affected --target=test:types --exclude="examples/**" --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx affected --target=test:unit --exclude="examples/**,e2e/**" --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
Bug Fixes
<Outlet />is rendered inside pending, error, or not-found components across React, Solid, and Vue Router.<Outlet />usage during loading, error, and not-found states.Tests
<Outlet />rendering scenarios across all supported frameworks.