feat(core): add tagsSplitDeduplication option for barrel + shared type extraction#3558
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (66)
📒 Files selected for processing (27)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (9)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughAdds ChangesShared-type deduplication and root barrel for tags-split mode
Sequence DiagramsequenceDiagram
participant User as orval config
participant FetchPkg as generateFetchHeader
participant ClientHeader as generateClientHeader
participant TargetTags as generateTargetForTags
participant SplitWriter as writeSplitTagsMode
User->>FetchPkg: tagsSplitDeduplication: true
FetchPkg-->>ClientHeader: { implementation: '', sharedTypes: HTTP_STATUS_CODE_SHARED_TYPES }
ClientHeader-->>TargetTags: { implementation, sharedTypes }
TargetTags->>TargetTags: deduplicationActive=true → set sharedTypes on tag output
TargetTags-->>SplitWriter: tags with sharedTypes arrays
SplitWriter->>SplitWriter: collect unique sharedTypes across all tags
SplitWriter->>SplitWriter: inject import type { } from common-types into each tag
SplitWriter-->>SplitWriter: write common-types.ts
SplitWriter-->>SplitWriter: write index.ts barrel (if indexFiles && !workspace)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for generating a root-level barrel index.ts for TAGS_SPLIT outputs when indexFiles is enabled, updating snapshots and tests accordingly.
Changes:
- Generate a root
index{ext}file exporting each per-tag module in tags-split mode (when enabled and not in workspace mode). - Add/refresh snapshot fixtures to include the new root
index.tsacross multiple clients/configurations. - Add unit tests validating creation/non-creation and returned-path inclusion for the new barrel file.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/snapshots/zod/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/vue-query/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/vue-query/http-client-fetch/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/vue-query/http-client-fetch-with-include-http-response-return-type/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/swr/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/swr/http-client-fetch/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/swr/http-client-fetch-with-include-http_status_return-type/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/svelte-query/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/svelte-query/http-client-fetch/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/svelte-query/http-client-fetch-with-include-http-response-return-type/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/react-query/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/react-query/issue-3301-include-unreferenced-schemas/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/react-query/issue-3269/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/react-query/issue-3066/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/react-query/invalidates-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/react-query/http-client-fetch/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/react-query/http-client-fetch-with-include-http-response-return-type/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/mock/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/mock/faker-array-items-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/mock/enums-inline-tags-split-native/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/hono/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/hono/petstore-tags-split-with-zod-mutator/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/hono/petstore-tags-split-with-handlers/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/fetch/zod-schema-response-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/fetch/zod-schema-response-suffix-tags-split/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/fetch/zod-schema-response-suffix-tags-split-no-runtime-validation/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/fetch/reviver/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/fetch/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/fetch/dateParams/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/default/regressions/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/default/multiple-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/default/issue-2998/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/default/index-mock-file/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/default/file-extension-tags-split/index.generated.ts | Snapshot adds barrel output matching the configured generated file extension. |
| tests/snapshots/axios/petstore-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/axios/petstore-tags-split-mutator/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| tests/snapshots/angular/tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output (Angular service suffix). |
| tests/snapshots/angular/issue-3103/index.ts | Snapshot adds a root barrel export for this scenario. |
| tests/snapshots/angular/http-resource-both-tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output (Angular service suffix). |
| tests/snapshots/angular-query/tags-split/index.ts | Snapshot now includes root barrel exports for tags-split output. |
| packages/core/src/writers/split-tags-mode.ts | Implements root barrel index{ext} generation in tags-split mode and returns it in the file list. |
| packages/core/src/writers/split-tags-mode.test.ts | Adds unit tests for writing (or not writing) the root barrel index and returning its path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/writers/split-tags-mode.test.ts (1)
60-123: ⚡ Quick winAdd explicit workspace-mode coverage for root barrel suppression.
Line 338 in the writer adds a
!output.workspacegate, but this suite only variesindexFiles. Add a test withindexFiles: trueandworkspaceset, then assert rootindex.tsis neither written nor returned.Suggested test addition
+ it('does not write index.ts when workspace is set', async () => { + const target = path.join(tmpDir, 'petstore.ts'); + const props = { + ...createSplitModeProps(target), + output: createSplitModeOutput(target, { + mode: OutputMode.TAGS_SPLIT, + indexFiles: true, + workspace: 'my-workspace', + }), + }; + + const paths = await writeSplitTagsMode({ ...props, needSchema: false }); + + const indexPath = path.join(tmpDir, 'index.ts'); + expect(paths).not.toContain(indexPath); + expect(fs.existsSync(indexPath)).toBe(false); + });🤖 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/core/src/writers/split-tags-mode.test.ts` around lines 60 - 123, The test suite for writeSplitTagsMode lacks coverage for when output.workspace is true; add a new test case alongside the existing 'writes index.ts when indexFiles is true' that sets output: createSplitModeOutput(..., { mode: OutputMode.TAGS_SPLIT, indexFiles: true, workspace: true }) and then assert that the root index.ts path is neither created on disk (fs.existsSync(indexPath) is false) nor included in the returned paths array (expect(paths).not.toContain(indexPath)); locate the test file split-tags-mode.test.ts and the helper usage around createSplitModeOutput/writeSplitTagsMode to insert this case so the !output.workspace gate in writeSplitTagsMode is exercised.
🤖 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.
Nitpick comments:
In `@packages/core/src/writers/split-tags-mode.test.ts`:
- Around line 60-123: The test suite for writeSplitTagsMode lacks coverage for
when output.workspace is true; add a new test case alongside the existing
'writes index.ts when indexFiles is true' that sets output:
createSplitModeOutput(..., { mode: OutputMode.TAGS_SPLIT, indexFiles: true,
workspace: true }) and then assert that the root index.ts path is neither
created on disk (fs.existsSync(indexPath) is false) nor included in the returned
paths array (expect(paths).not.toContain(indexPath)); locate the test file
split-tags-mode.test.ts and the helper usage around
createSplitModeOutput/writeSplitTagsMode to insert this case so the
!output.workspace gate in writeSplitTagsMode is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d523ef79-31cc-44a2-9e0b-a82b7bd0ac3e
📒 Files selected for processing (42)
packages/core/src/writers/split-tags-mode.test.tspackages/core/src/writers/split-tags-mode.tstests/__snapshots__/angular-query/tags-split/index.tstests/__snapshots__/angular/http-resource-both-tags-split/index.tstests/__snapshots__/angular/issue-3103/index.tstests/__snapshots__/angular/tags-split/index.tstests/__snapshots__/axios/petstore-tags-split-mutator/index.tstests/__snapshots__/axios/petstore-tags-split/index.tstests/__snapshots__/default/file-extension-tags-split/index.generated.tstests/__snapshots__/default/index-mock-file/index.tstests/__snapshots__/default/issue-2998/index.tstests/__snapshots__/default/multiple-tags-split/index.tstests/__snapshots__/default/regressions/index.tstests/__snapshots__/fetch/dateParams/index.tstests/__snapshots__/fetch/petstore-tags-split/index.tstests/__snapshots__/fetch/reviver/index.tstests/__snapshots__/fetch/zod-schema-response-suffix-tags-split-no-runtime-validation/index.tstests/__snapshots__/fetch/zod-schema-response-suffix-tags-split/index.tstests/__snapshots__/fetch/zod-schema-response-tags-split/index.tstests/__snapshots__/hono/petstore-tags-split-with-handlers/index.tstests/__snapshots__/hono/petstore-tags-split-with-zod-mutator/index.tstests/__snapshots__/hono/petstore-tags-split/index.tstests/__snapshots__/mock/enums-inline-tags-split-native/index.tstests/__snapshots__/mock/faker-array-items-tags-split/index.tstests/__snapshots__/mock/petstore-tags-split/index.tstests/__snapshots__/react-query/http-client-fetch-with-include-http-response-return-type/index.tstests/__snapshots__/react-query/http-client-fetch/index.tstests/__snapshots__/react-query/invalidates-tags-split/index.tstests/__snapshots__/react-query/issue-3066/index.tstests/__snapshots__/react-query/issue-3269/index.tstests/__snapshots__/react-query/issue-3301-include-unreferenced-schemas/index.tstests/__snapshots__/react-query/petstore-tags-split/index.tstests/__snapshots__/svelte-query/http-client-fetch-with-include-http-response-return-type/index.tstests/__snapshots__/svelte-query/http-client-fetch/index.tstests/__snapshots__/svelte-query/petstore-tags-split/index.tstests/__snapshots__/swr/http-client-fetch-with-include-http_status_return-type/index.tstests/__snapshots__/swr/http-client-fetch/index.tstests/__snapshots__/swr/petstore-tags-split/index.tstests/__snapshots__/vue-query/http-client-fetch-with-include-http-response-return-type/index.tstests/__snapshots__/vue-query/http-client-fetch/index.tstests/__snapshots__/vue-query/petstore-tags-split/index.tstests/__snapshots__/zod/petstore-tags-split/index.ts
cdb6901 to
0e132ee
Compare
|
where are we with this PR? |
|
@melloware still waiting on a response to #3553 (comment) |
|
|
0e132ee to
73df503
Compare
Generate an index.ts barrel at the target root that re-exports all per-tag implementation files when using mode: 'tags-split' with indexFiles: true (the default). This is consistent with the existing mock barrel behavior (mock.indexMockFiles). The barrel is skipped when output.workspace is set, since the workspace barrel in write-specs.ts already serves that purpose. Closes orval-labs#3553
3177fa9 to
ca3d6c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/src/writers/split-tags-mode.test.ts (1)
416-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a positive test case for
common-types.tsemission.The test verifies that
common-types.tsis not written when no shared types exist, which is correct. However, the test suite lacks a complementary positive case that verifies the file is written and contains the expected content when shared types are present (e.g., when using a fetch client that emits HTTP status code types).A positive test would:
- Configure output with
client: 'fetch'to trigger shared-type emission- Assert that
common-types.tsis written and included in returned paths- Optionally verify the file contains expected type declarations (e.g.,
HTTPStatusCode)This would strengthen confidence that the deduplication mechanism correctly extracts and writes shared types.
🤖 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/core/src/writers/split-tags-mode.test.ts` around lines 416 - 432, Add a new test case (similar structure to the existing test starting at line 416) that verifies the positive scenario: when shared types are present, common-types.ts is written and included in the output paths. In this new test, configure the output props with client set to 'fetch' to trigger shared-type emission, then assert that the commonTypesPath is included in the returned paths array and that fs.existsSync confirms the file was created. Optionally, read the file content and verify it contains expected type declarations like HTTPStatusCode to strengthen validation that the deduplication mechanism correctly extracts and writes shared types.packages/core/src/writers/split-tags-mode.ts (1)
83-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a conflict check when deduplicating shared types by name.
Current dedup keeps the first declaration per
nameand silently drops later ones. Ifcode/exporteddiverge for the same name, generatedcommon-typescan become inconsistent.💡 Proposed fix
- const collectedSharedTypes: SharedTypeDeclaration[] = []; - const seenSharedTypeNames = new Set<string>(); + const collectedSharedTypes: SharedTypeDeclaration[] = []; + const sharedTypesByName = new Map<string, SharedTypeDeclaration>(); for (const [, target] of tagEntries) { if (!target.sharedTypes) continue; for (const t of target.sharedTypes) { - if (!seenSharedTypeNames.has(t.name)) { - seenSharedTypeNames.add(t.name); - collectedSharedTypes.push(t); - } + const existing = sharedTypesByName.get(t.name); + if (!existing) { + sharedTypesByName.set(t.name, t); + collectedSharedTypes.push(t); + continue; + } + if (existing.code !== t.code || existing.exported !== t.exported) { + throw new Error( + `Conflicting shared type declaration for "${t.name}" in tags-split generation.`, + ); + } } }🤖 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/core/src/writers/split-tags-mode.ts` around lines 83 - 93, The deduplication logic in the loop that processes target.sharedTypes currently silently discards subsequent declarations when a shared type name has already been encountered. When deduplicating by name using the seenSharedTypeNames set and collectedSharedTypes array, add a conflict check that compares the code and exported properties of the new declaration against the previously collected one. If these properties differ between declarations with the same name, the conflict should be explicitly handled (such as logging a warning, throwing an error, or applying a conflict resolution strategy) to prevent inconsistent common-types generation. This check should occur when a duplicate name is detected and before continuing to the next iteration.
🤖 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 `@docs/content/docs/zh/reference/configuration/output.mdx`:
- Around line 142-147: The documentation for `tagsSplitDeduplication`
configuration option is missing an important note about its interaction with
workspace mode. Update the documentation section for `tagsSplitDeduplication` to
clearly state that this option is automatically suppressed and has no effect
when `output.workspace` is enabled, since the implementation applies the logic
`deduplicationEnabled = output.tagsSplitDeduplication && !output.workspace`. Add
a warning or note that explicitly tells users that enabling
`tagsSplitDeduplication` while workspace mode is active will not produce the
expected behavior.
In `@packages/core/src/writers/split-tags-mode.ts`:
- Around line 125-133: The code imports all shared type names from
target.sharedTypes without filtering for those actually exported by
common-types. Since common-types conditionally omits the export keyword for some
declarations, this generates invalid imports for non-exported symbols. Filter
target.sharedTypes to only include types that meet the export conditions
(matching the logic at lines 409-412) before mapping their names and
constructing the import statement in the implementationData variable.
---
Nitpick comments:
In `@packages/core/src/writers/split-tags-mode.test.ts`:
- Around line 416-432: Add a new test case (similar structure to the existing
test starting at line 416) that verifies the positive scenario: when shared
types are present, common-types.ts is written and included in the output paths.
In this new test, configure the output props with client set to 'fetch' to
trigger shared-type emission, then assert that the commonTypesPath is included
in the returned paths array and that fs.existsSync confirms the file was
created. Optionally, read the file content and verify it contains expected type
declarations like HTTPStatusCode to strengthen validation that the deduplication
mechanism correctly extracts and writes shared types.
In `@packages/core/src/writers/split-tags-mode.ts`:
- Around line 83-93: The deduplication logic in the loop that processes
target.sharedTypes currently silently discards subsequent declarations when a
shared type name has already been encountered. When deduplicating by name using
the seenSharedTypeNames set and collectedSharedTypes array, add a conflict check
that compares the code and exported properties of the new declaration against
the previously collected one. If these properties differ between declarations
with the same name, the conflict should be explicitly handled (such as logging a
warning, throwing an error, or applying a conflict resolution strategy) to
prevent inconsistent common-types generation. This check should occur when a
duplicate name is detected and before continuing to the next iteration.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 821735d4-dfa4-42d4-874e-e068bddf5364
📒 Files selected for processing (20)
docs/content/docs/reference/configuration/output.mdxdocs/content/docs/zh/reference/configuration/output.mdxpackages/angular/src/http-client.test.tspackages/angular/src/http-resource.test.tspackages/core/src/test-utils/context.tspackages/core/src/test-utils/split-modes.tspackages/core/src/types.tspackages/core/src/writers/split-tags-mode.test.tspackages/core/src/writers/split-tags-mode.tspackages/core/src/writers/target-tags.tspackages/core/src/writers/target.tspackages/fetch/src/index.tspackages/mcp/src/index.tspackages/mock/src/faker/getters/combine.test.tspackages/orval/src/client.tspackages/orval/src/utils/options.tspackages/query/src/frameworks/react.test.tspackages/query/src/index.tspackages/solid-start/src/index.test.tspackages/swr/src/index.ts
✅ Files skipped from review due to trivial changes (3)
- packages/mock/src/faker/getters/combine.test.ts
- packages/core/src/test-utils/context.ts
- docs/content/docs/reference/configuration/output.mdx
…e extraction Add a new `tagsSplitDeduplication` output option (default: false) that extracts shared infrastructure types (e.g. HTTPStatusCode*) emitted by client header builders into a `common-types.ts` file, then generates a barrel `index.ts` with named re-exports for public types. When enabled (with `indexFiles: true`): - Shared types are collected across all per-tag files and deduplicated - Extracted to `<commonTypesFileName>.ts` (default: 'common-types') - Per-tag files import shared types from the common file - Barrel uses named re-exports for exported types + `export *` for tags When disabled (default): behavior is identical to before — shared types are inlined per-tag, no barrel, no extraction. The header builder API is backward compatible: `ClientHeaderBuilder` return type widens from `string` to `string | HeaderResult`. Existing builders returning `string` continue to work unchanged. Currently only `generateFetchHeader` emits `sharedTypes` (HTTPStatusCode* family). The extraction mechanism is generic — other header builders can adopt it incrementally. Remove stale barrel snapshots from the previous naive implementation (gated on `indexFiles` alone). The barrel now requires both `indexFiles` and `tagsSplitDeduplication` to be `true`. Related design discussion: orval-labs#3553 Closes orval-labs#3553
ca3d6c5 to
4e8a1bf
Compare
|
@melloware the PR is ready. |
Problem
Fix #3553, #3108
When using
mode: "tags-split", orval generates per-tag subdirectories but creates no barrelindex.tsat the target root. A naive barrel (export *from each tag) fails because fetch-based clients emitHTTPStatusCode*types into every tag file, causingTS2308duplicate export errors.Solution
New
tagsSplitDeduplicationoption (default:false) that:<commonTypesFileName>.ts(default:common-types.ts)index.tswith named re-exports for public shared types +export *for per-tag filesConfiguration
Resulting structure
Behavior matrix
tagsSplitDeduplicationindexFilesfalse(default)truetruecommon-types.ts+ barrel with named re-exportstruetrueexport *only (no common-types)truefalsecommon-types.tsextracted, no barreltruefalseSuppressed when
output.workspaceis set (workspace barrel handles it).Implementation
ClientHeaderBuilderreturn type widened fromstringtostring | HeaderResult(backward compatible)generateFetchHeadernow returnssharedTypesforHTTPStatusCode*instead of inlining themgenerateClientHeadernormalizes both return forms intoGeneratorClientExtragenerateTargetForTagsinlines shared types back when dedup is off, passes them through when onwriteSplitTagsModecollects, deduplicates, writescommon-types.ts, generates barrelCurrently only
generateFetchHeaderemitssharedTypes. The mechanism is generic — other header builders (angularHttpClientOptions, etc.) can adopt it incrementally in follow-up PRs.Design discussion
See issue comment for the full design discussion including:
export *barrels fail (HTTPStatusCode collision in 13 fetch-based configs)indexFilesCloses #3553
Summary by CodeRabbit
tagsSplitDeduplication(defaultfalse) fortags-splitmode to extract shared infrastructure types into acommon-types.tsfile.commonTypesFileName(default'common-types') to control the shared-types filename.indexFiles: true(and not inworkspacemode), generates a rootindex.tsbarrel with named + re-export entries.