fix(orval): re-export client extra files from the tags-split barrel - #3815
Draft
the-ult wants to merge 4 commits into
Draft
fix(orval): re-export client extra files from the tags-split barrel#3815the-ult wants to merge 4 commits into
the-ult wants to merge 4 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds export parsing and deduplicated barrel augmentation. ChangesTags-split barrel generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant writeSpecs
participant addExtraFilesToTagsSplitBarrel
participant appendReExportsToBarrel
participant TagsSplitBarrel
writeSpecs->>addExtraFilesToTagsSplitBarrel: process generated extra files
addExtraFilesToTagsSplitBarrel->>appendReExportsToBarrel: pass specifiers and file contents
appendReExportsToBarrel->>TagsSplitBarrel: append deduplicated re-exports
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
This was referenced Aug 5, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/orval/src/utils/barrel.ts`:
- Around line 173-177: Update the canonical selection logic around isShared and
canonical so each shared type or value name is associated with the first parsed
entry that declares it, rather than always using parsed[0]. Use those per-name
canonical entries when generating explicit exports, and add a regression case
covering a unique first entry followed by two entries exporting the same name.
In `@packages/orval/src/write-specs.ts`:
- Around line 105-107: Update the specifier निर्माण in write-specs so the
configured output.fileExtension is removed as a full suffix before appending
importExtension, rather than stripping only the last dot segment; this affects
the specifier path built from upath.relativeSafe(dirname, filePath) in
write-specs. Reuse the same suffix-removal approach already used by the
workspace-barrel logic in the same module, so a value like .generated.ts
produces pets.generated instead of pets.generated.generated.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d979cf0f-daef-4440-a346-115cce194dc9
⛔ Files ignored due to path filters (12)
tests/__snapshots__/angular/http-resource-both-tags-split-barrel/health/health.resource.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/health/health.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/createPetsBody.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/error.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/pet.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/searchPetsBody.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/model/searchPetsBodyStatus.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/pets/pets.resource.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-both-tags-split-barrel/pets/pets.service.tsis excluded by!**/__snapshots__/**
📒 Files selected for processing (4)
packages/orval/src/utils/barrel.test.tspackages/orval/src/utils/barrel.tspackages/orval/src/write-specs.tstests/configs/angular.config.ts
Comment on lines
+173
to
+177
| const isShared = (name: string) => | ||
| (nameCounts.get(name) ?? 0) > 1 && !alreadyNamed.has(name); | ||
| const canonical = parsed[0]; | ||
| const sharedTypes = [...canonical.names.types].filter(isShared).sort(); | ||
| const sharedValues = [...canonical.names.values].filter(isShared).sort(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Select a canonical entry for each shared name.
canonical always uses parsed[0]. If the first entry exports only unique names, but two later entries export Shared, both wildcard exports remain ambiguous and TypeScript reports TS2308.
Track the first entry that declares each shared name. Emit explicit exports from that entry. Add a regression case with one unique first entry and two later entries that export the same name.
🤖 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/orval/src/utils/barrel.ts` around lines 173 - 177, Update the
canonical selection logic around isShared and canonical so each shared type or
value name is associated with the first parsed entry that declares it, rather
than always using parsed[0]. Use those per-name canonical entries when
generating explicit exports, and add a regression case covering a unique first
entry followed by two entries exporting the same name.
Comment on lines
+105
to
+107
| specifier: | ||
| upath.relativeSafe(dirname, filePath).replace(/\.[^./]+$/, '') + | ||
| importExtension, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the complete configured file extension.
If output.fileExtension is .generated.ts, this code converts pets.generated.ts to pets.generated, then appends .generated. The barrel imports pets.generated.generated.
Remove extension as a complete suffix before appending importExtension. The workspace-barrel logic at Lines 847-875 already uses this pattern.
Proposed fix
- specifier:
- upath.relativeSafe(dirname, filePath).replace(/\.[^./]+$/, '') +
- importExtension,
+ specifier:
+ (upath.relativeSafe(dirname, filePath).endsWith(extension)
+ ? upath
+ .relativeSafe(dirname, filePath)
+ .slice(0, -extension.length)
+ : upath.relativeSafe(dirname, filePath).replace(/\.[^./]+$/, '')) +
+ importExtension,📝 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.
Suggested change
| specifier: | |
| upath.relativeSafe(dirname, filePath).replace(/\.[^./]+$/, '') + | |
| importExtension, | |
| specifier: | |
| (upath.relativeSafe(dirname, filePath).endsWith(extension) | |
| ? upath | |
| .relativeSafe(dirname, filePath) | |
| .slice(0, -extension.length) | |
| : upath.relativeSafe(dirname, filePath).replace(/\.[^./]+$/, '')) + | |
| importExtension, |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 107-107: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/orval/src/write-specs.ts` around lines 105 - 107, Update the
specifier निर्माण in write-specs so the configured output.fileExtension is
removed as a full suffix before appending importExtension, rather than stripping
only the last dot segment; this affects the specifier path built from
upath.relativeSafe(dirname, filePath) in write-specs. Reuse the same
suffix-removal approach already used by the workspace-barrel logic in the same
module, so a value like .generated.ts produces pets.generated instead of
pets.generated.generated.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an Angular tags-split + tagsSplitDeduplication client-barrel gap by appending re-exports for client “extra files” (notably per-tag *.resource.ts) to the generated <clientDir>/index.ts, using actual written file paths and adding TS2308-safe explicit re-exports for duplicated boilerplate symbols.
Changes:
- Add a
writeSpecsstep to append extra-file re-exports to thetags-splitclient barrel after extra files are written. - Introduce
appendReExportsToBarrel(+readExportedNames) to dedupe appended exports and avoid TS2308 by explicitly re-exporting duplicated symbols ahead of wildcard exports. - Add an Angular regression fixture/config and unit tests for the new barrel-append behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/configs/angular.config.ts | Adds a regression config that exercises tagsSplitDeduplication + Angular retrievalClient: 'both' barrel behavior. |
| tests/snapshots/angular/http-resource-both-tags-split-barrel/** | New generated fixture proving the barrel includes *.resource.ts and TS2308-safe explicit exports. |
| packages/orval/src/write-specs.ts | Appends extra-file exports into the tags-split client barrel after writing client extra files. |
| packages/orval/src/utils/barrel.ts | Adds appendReExportsToBarrel and helpers to append/dedupe exports and mitigate TS2308. |
| packages/orval/src/utils/barrel.test.ts | Adds unit tests for exported-name parsing and barrel append/idempotency behavior. |
Suppressed comments (1)
packages/orval/src/utils/barrel.ts:177
- The TS2308 mitigation currently picks
parsed[0]as the canonical module and only explicitly re-exports shared names that are declared in that one file. If the duplicated symbol set exists only among later entries (e.g. entries[1] + entries[2]),sharedTypes/sharedValuesbecome empty and the appendedexport *lines can still trigger TS2308. Canonical selection (or explicit export selection) should be based on which module(s) actually declare the duplicated names, not array position.
const isShared = (name: string) =>
(nameCounts.get(name) ?? 0) > 1 && !alreadyNamed.has(name);
const canonical = parsed[0];
const sharedTypes = [...canonical.names.types].filter(isShared).sort();
const sharedValues = [...canonical.names.values].filter(isShared).sort();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
+171
| const alreadyNamed = new Set<string>(); | ||
| for (const line of existingContent.split(/\r?\n/)) { | ||
| const names = line.match(NAMED_RE_EXPORT_LINE)?.[1]; | ||
| if (!names) continue; | ||
| for (const name of names.split(',')) { | ||
| alreadyNamed.add( | ||
| name | ||
| .trim() | ||
| .split(/\s+as\s+/) | ||
| .pop()!, | ||
| ); | ||
| } | ||
| } |
@orval/angular
@orval/axios
@orval/core
@orval/effect
@orval/fetch
@orval/hono
@orval/mcp
@orval/mock
orval
@orval/query
@orval/solid-start
@orval/swr
@orval/zod
commit: |
the-ult
marked this pull request as draft
August 5, 2026 13:25
In `tags-split` mode with `tagsSplitDeduplication`, `writeSplitTagsMode` emits `<clientDir>/index.ts` — the only complete client entry point — but builds it from the per-tag implementation files alone. Client extra files are produced by the client builder, so Angular's `retrievalClient: 'both'` left every per-tag `*.resource.ts` unreachable from the barrel: client/pets/pets.service.ts client/pets/pets.resource.ts client/index.ts -> export * from './pets/pets.service'; # resource missing Consumers wanting the resource API had to import per-tag paths directly, which is exactly what a module-boundary-enforced monorepo forbids. `builder.extraFiles` is already in scope in `writeSplitTagsMode`, so the re-exports are composed into the barrel content before it is written rather than patched in afterwards. They are derived from the emitted paths, not from tag names: a mutation-only tag produces no resource file, and a name-derived barrel would export a file that is never written. Files outside the client directory belong to another barrel and are left alone. Each `*.resource.ts` carries its own copy of the shared httpResource boilerplate, so plain wildcards make those names ambiguous — TypeScript reports TS2308 and recommends exactly this remedy. The names are declared by the generator that emits them (`ClientFileBuilder.sharedExports`), built from the same constants the templates interpolate, and re-exported from a single file ahead of the wildcards. Ownership is per name rather than per file, so a generator emitting some of its shared declarations conditionally stays correct. Declared rather than inferred from the generated source: inference cannot tell intentional boilerplate from an accidental collision between two tags, and would silently resolve the latter to one arbitrary file — a wrong type at the call site in place of a build failure. Generators whose extra files carry no repeated declarations (hono, mcp) declare nothing and are unaffected. New `httpResourceBothTagsSplitBarrel` fixture covers it; the generated-output typecheck gate passes for all 16 clients. `http-resource.test.ts` asserts the declaration against the names two generated resource files actually share, so a new shared declaration cannot be added without being listed. No existing snapshot changed — no prior fixture combined `tagsSplitDeduplication` with a client emitting extra files. Workspace output still has the same gap: it emits no client barrel at all, and its workspace barrel omits extra files for the same ordering reason. The shared helper sits in core's utils so closing that is additive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
the-ult
force-pushed
the
fix/core-tags-split-barrel-angular-resource
branch
from
August 5, 2026 14:58
c7cb871 to
88197d4
Compare
The barrel composition added every client extra file, for every client. Under `tags-split` with `tagsSplitDeduplication` that pulled in files no consumer should import through a barrel. The `mcp` generator emits a `server.ts` that calls `server.connect(transport)` at module top level, so importing the barrel would start a stdio server as a side effect. A generator now opts in per file with `ClientFileBuilder.barrelExport`. Angular sets it on its resource files. `hono` and `mcp` do not set it, so their output is unchanged. Also addresses review feedback: - Extract `pathWithoutExtension` and use it in place of a second copy of the extension-strip regex. - Move the path to specifier step into `buildBarrelReExports`, which already owns barrel line construction. - Replace the ownership bookkeeping with a single pass over entries. - Keep one canonical TS2308 explanation and cross-reference it. - Correct the comment that presented wildcard ordering as required. An explicit re-export shadows `export *` at any position. The order only keeps the output stable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
What
In
tags-splitmode withtagsSplitDeduplication,writeSplitTagsModeemits<clientDir>/index.ts. That barrel is the only complete client entry point, but it was built from the per-tag implementation files alone. Client extra files come from the client builder, so with Angular'sretrievalClient: 'both'every per-tag*.resource.tswas missing:To reach the resource API, consumers had to bypass the barrel and import per-tag paths directly.
Why this fix
builder.extraFilesis already in scope inwriteSplitTagsMode, so the re-exports are composed into the barrel before it is written.A generator opts in per file, through the new
ClientFileBuilder.barrelExportflag. Angular sets it on its resource files; other generators do not, so their extra files stay out.mcp, for example, emits aserver.tsthat callsserver.connect(transport)at module top level — that must never run as an import side effect.The re-exports come from the emitted file paths, not from tag names: a generator emits a resource file only for tags with retrieval operations, so a name-derived barrel would export a file that was never written. Files outside the client directory belong to another barrel and are left alone.
Plain wildcards are not enough. Each
*.resource.tsrepeats the same shared boilerplate (ResourceState,toResourceState,OrvalHttpResourceOptions, and so on), so wildcard-exporting two of them makes every repeated name ambiguous and TypeScript reports TS2308. Each repeated name is therefore re-exported by name from one owning file, which shadows the wildcards. The generator declares those names throughClientFileBuilder.sharedExports. The writer does not infer them, because inference cannot tell intentional boilerplate from an accidental name collision between two tags — the first must resolve to one file, the second is a defect and must stay an error.Resulting barrel:
Types and values are re-exported separately, because a type re-exported without the
typemodifier is an error underverbatimModuleSyntax, which Angular enables by default. Line order is fixed only to keep output stable; TypeScript does not depend on it.User-visible changes
Under
tags-split+tagsSplitDeduplication+indexFiles, the client barrel also exports client extra files that opt in. Today only Angular resource files do.Tests
httpResourceBothTagsSplitBarrelfixture intests/configs/angular.config.ts.barrel-re-exports.test.ts— the helper: per-name ownership, names the barrel already exports by name, entries without shared exports.split-tags-mode.test.ts— the composition: a tag without an extra file is not exported, a file that does not setbarrelExportis not exported, files outside the client directory are ignored.http-resource.test.ts— asserts the declaredsharedExportsagainst the names two generated resource files actually share, so a new shared declaration cannot be added without being listed.Known, out of scope
Workspace output has the same hole:
deduplicationEnabledistagsSplitDeduplication && !output.workspace, so no client barrel is written at all. The new helper sits in@orval/core/src/utils/, so that follow-up is additive.Verification
build:release,typecheck,lint,format:check,test,test:snapshots— all green. No committed fixture or sample output changed.🤖 Generated with Claude Code