fix: compiler handles type-only exports #6199
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new route (/type-only-reexport) and test fixtures that exercise type-only barrel re-exports, introduces a logging middleware and type-only typedefs, and updates the server-fn plugin to tolerate missing/empty module code during build. E2E and unit tests validate the change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Browser (Client)
participant Router as React Router (Client)
participant Server as Start Server API
participant Middleware as loggingMiddleware
participant ServerFnPlugin as create-server-fn-plugin
Note over ServerFnPlugin, Middleware: Two flows: build-time ingestion and runtime call
rect rgb(240, 248, 255)
Note right of ServerFnPlugin: Build-time: tolerate empty modules
ServerFnPlugin->>ServerFnPlugin: load module (loaded.code ?? '')
ServerFnPlugin->>ServerFnPlugin: ingestModule (empty OK)
end
rect rgb(245, 255, 240)
Browser->>Router: navigate /type-only-reexport
Router->>Server: call server loader (server function)
Server->>Middleware: invoke loggingMiddleware
Middleware->>Server: next() -> actual server function
Server->>Browser: return loader data (message)
Browser->>Router: render TypeOnlyReexportPage with loader data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
|
View your CI Pipeline Execution ↗ for commit 6a35a2d
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/start-plugin-core/tests/compiler.test.ts (1)
395-414: Move test into a describe block for consistency.The test is placed at the top level, while all other tests in the file are organized within
describeblocks. This breaks the file's organizational structure.🔎 Suggested reorganization
-test('ingestModule handles empty code gracefully', () => { - const compiler = new ServerFnCompiler({ - env: 'client', - directive: 'use server', - lookupKinds: new Set(['ServerFn']), - lookupConfigurations: [], - loadModule: async () => {}, - resolveId: async (id) => id, - }) - - // Should not throw when ingesting empty module - expect(() => { - compiler.ingestModule({ code: '', id: 'empty-types.ts' }) - }).not.toThrow() - - // Should also handle whitespace-only modules - expect(() => { - compiler.ingestModule({ code: ' \n\t ', id: 'whitespace.ts' }) - }).not.toThrow() -}) +describe('ingestModule with type-only modules', () => { + test('handles empty code gracefully', () => { + const compiler = new ServerFnCompiler({ + env: 'client', + directive: 'use server', + lookupKinds: new Set(['ServerFn']), + lookupConfigurations: [], + loadModule: async () => {}, + resolveId: async (id) => id, + }) + + // Should not throw when ingesting empty module + expect(() => { + compiler.ingestModule({ code: '', id: 'empty-types.ts' }) + }).not.toThrow() + + // Should also handle whitespace-only modules + expect(() => { + compiler.ingestModule({ code: ' \n\t ', id: 'whitespace.ts' }) + }).not.toThrow() + }) +})Optionally, you could enhance the test to verify that module info is actually created and cached, not just that it doesn't throw:
const result = compiler.ingestModule({ code: '', id: 'empty-types.ts' }) expect(result.info).toBeDefined() expect(result.info.id).toBe('empty-types.ts') expect(result.info.bindings.size).toBe(0) expect(result.info.exports.size).toBe(0)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/react-start/basic/src/routeTree.gen.tse2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/shared-lib/index.tse2e/react-start/basic/src/shared-lib/middleware.tse2e/react-start/basic/src/shared-lib/typedefs/actions.tse2e/react-start/basic/src/shared-lib/typedefs/index.tse2e/react-start/basic/tests/type-only-reexport.spec.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/start-plugin-core/tests/compiler.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-plugin-core/tests/compiler.test.tse2e/react-start/basic/src/shared-lib/middleware.tse2e/react-start/basic/tests/type-only-reexport.spec.tse2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/shared-lib/typedefs/actions.tse2e/react-start/basic/src/shared-lib/index.tse2e/react-start/basic/src/shared-lib/typedefs/index.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tse2e/react-start/basic/src/routeTree.gen.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/tests/compiler.test.tse2e/react-start/basic/src/shared-lib/middleware.tse2e/react-start/basic/tests/type-only-reexport.spec.tse2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/shared-lib/typedefs/actions.tse2e/react-start/basic/src/shared-lib/index.tse2e/react-start/basic/src/shared-lib/typedefs/index.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tse2e/react-start/basic/src/routeTree.gen.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/basic/tests/type-only-reexport.spec.tse2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-start/basic/tests/type-only-reexport.spec.tse2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/shared-lib/index.tse2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/basic/tests/type-only-reexport.spec.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/react-start/basic/src/routes/type-only-reexport.tsxe2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/basic/src/routeTree.gen.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/basic/src/routeTree.gen.ts
🧬 Code graph analysis (1)
e2e/react-start/basic/src/routes/type-only-reexport.tsx (2)
e2e/react-start/basic/src/shared-lib/middleware.ts (1)
loggingMiddleware(7-10)packages/router-core/src/ssr/tsrScript.ts (1)
p(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (7)
e2e/react-start/basic/src/shared-lib/typedefs/actions.ts (1)
1-11: LGTM!The type-only exports are correctly structured for testing the compiler's handling of modules that compile to empty JavaScript. The
biome-ignorecomment is appropriate for the genericany[]usage in the test fixture.e2e/react-start/basic/src/shared-lib/typedefs/index.ts (1)
1-4: LGTM!Clean barrel file for type-only re-exports.
e2e/react-start/basic/src/shared-lib/middleware.ts (1)
1-10: LGTM!Clean middleware implementation using the TanStack Start API correctly. The logging and forwarding to
next()is appropriate for testing type-only re-export scenarios.e2e/react-start/basic/src/routeTree.gen.ts (1)
1-1019: Skipping review of auto-generated file.This file is auto-generated by TanStack Router (as noted in the header comment). Based on established practices, auto-generated route tree files should not be manually reviewed or modified.
e2e/react-start/basic/src/routes/type-only-reexport.tsx (2)
16-28: LGTM!The server function and route setup correctly demonstrate the use of middleware from a barrel file that re-exports from type-only modules. The implementation properly tests the compiler's ability to handle type-only module re-exports.
30-42: LGTM!The component correctly uses
Route.useLoaderData()and renders the test data with appropriate test IDs for E2E validation.packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (1)
144-148: Good fix for handling type-only exports in build mode.The nullish coalescing operator correctly handles modules where
loaded.codeisnull(e.g., type-only exports that compile to empty modules). The comment clearly explains the rationale.The dev path using
fetchModuleand the capture plugin's transform hook already handles type-only exports correctly by design. Vite's transform hook always providescodeas a string, and theingestModulemethod is tested to handle empty code gracefully.
| * | ||
| * The loggingMiddleware is imported from ~/shared-lib, which re-exports from: | ||
| * - ./middleware (has runtime code) | ||
| * - ./types (has ONLY type exports - compiles to empty JS) |
There was a problem hiding this comment.
Fix documentation: path reference inconsistency.
The comment refers to ./types but the actual directory is ./typedefs (as seen in the import on line 3). Update the comment to match the actual directory name.
🔎 Proposed fix
- * - ./types (has ONLY type exports - compiles to empty JS)
+ * - ./typedefs (has ONLY type exports - compiles to empty JS)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * - ./types (has ONLY type exports - compiles to empty JS) | |
| * - ./typedefs (has ONLY type exports - compiles to empty JS) |
🤖 Prompt for AI Agents
In e2e/react-start/basic/src/routes/type-only-reexport.tsx around line 10, the
inline comment references "./types" but the actual import directory is
"./typedefs"; update the comment to use "./typedefs" so the path in the
documentation matches the real import.
| * | ||
| * This barrel file re-exports from both: | ||
| * 1. ./middleware - has runtime code (createMiddleware) | ||
| * 2. ./types - has ONLY type exports (compiles to empty JS) |
There was a problem hiding this comment.
Fix documentation: path reference inconsistency.
The comment refers to ./types but the actual export on line 12 uses ./typedefs. Update the comment to match the actual directory name.
🔎 Proposed fix
- * 2. ./types - has ONLY type exports (compiles to empty JS)
+ * 2. ./typedefs - has ONLY type exports (compiles to empty JS)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * 2. ./types - has ONLY type exports (compiles to empty JS) | |
| * 2. ./typedefs - has ONLY type exports (compiles to empty JS) |
🤖 Prompt for AI Agents
In e2e/react-start/basic/src/shared-lib/index.ts around line 6, the inline
comment mentions "./types" but the actual export on line 12 uses "./typedefs";
update the comment to reference "./typedefs" (or rename the export to match the
comment) so the documentation and code match, keeping naming consistent across
the file.
| * 1. ~/shared-lib/index.ts re-exports from ./middleware and ./types | ||
| * 2. ~/shared-lib/types/index.ts re-exports from ./actions.ts | ||
| * 3. ~/shared-lib/types/actions.ts contains ONLY type exports (no runtime code) |
There was a problem hiding this comment.
Fix documentation: path reference inconsistency.
The comments refer to ~/shared-lib/types but the actual directory is ~/shared-lib/typedefs. Update references to match the actual directory structure:
- Line 8:
./types→./typedefs - Line 9:
~/shared-lib/types/index.ts→~/shared-lib/typedefs/index.ts - Line 10:
~/shared-lib/types/actions.ts→~/shared-lib/typedefs/actions.ts
🔎 Proposed fix
* The scenario:
- * 1. ~/shared-lib/index.ts re-exports from ./middleware and ./types
- * 2. ~/shared-lib/types/index.ts re-exports from ./actions.ts
- * 3. ~/shared-lib/types/actions.ts contains ONLY type exports (no runtime code)
+ * 1. ~/shared-lib/index.ts re-exports from ./middleware and ./typedefs
+ * 2. ~/shared-lib/typedefs/index.ts re-exports from ./actions.ts
+ * 3. ~/shared-lib/typedefs/actions.ts contains ONLY type exports (no runtime code)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * 1. ~/shared-lib/index.ts re-exports from ./middleware and ./types | |
| * 2. ~/shared-lib/types/index.ts re-exports from ./actions.ts | |
| * 3. ~/shared-lib/types/actions.ts contains ONLY type exports (no runtime code) | |
| * 1. ~/shared-lib/index.ts re-exports from ./middleware and ./typedefs | |
| * 2. ~/shared-lib/typedefs/index.ts re-exports from ./actions.ts | |
| * 3. ~/shared-lib/typedefs/actions.ts contains ONLY type exports (no runtime code) |
🤖 Prompt for AI Agents
In e2e/react-start/basic/tests/type-only-reexport.spec.ts around lines 8 to 10,
the inline comments reference a non-existent ./types directory and
~/shared-lib/types files; update those path references to match the actual
directory ~/shared-lib/typedefs by changing "./types" to "./typedefs" on line 8,
"~/shared-lib/types/index.ts" to "~/shared-lib/typedefs/index.ts" on line 9, and
"~/shared-lib/types/actions.ts" to "~/shared-lib/typedefs/actions.ts" on line 10
so the comments reflect the real project structure.
schiller-manuel
force-pushed
the
fix-6198
branch
from
a276af0 to
6a35a2d
Compare
fixes #6198
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.