fix(angular): honor schemas.importPath and splitByTags in resource files - #3813
Open
the-ult wants to merge 7 commits into
Open
fix(angular): honor schemas.importPath and splitByTags in resource files#3813the-ult wants to merge 7 commits into
the-ult wants to merge 7 commits into
Conversation
`generateImportsForBuilder` owned the rule that maps a schema import to the module it is imported from: package specifier vs relative path, file extension vs none, tag subdirectory, zod suffix and basename, dedup key. Client generators that emit extra sibling files cannot call it — it is not exported from `@orval/core` — so Angular's `*.resource.ts` builder carried a copy that had drifted. Move the rule to `utils/schema-import-path.ts` as `resolveSchemaImportDependencies`, exported through the existing `./utils` barrel that `getSchemasImportPath` already uses for the same reason. `generateImportsForBuilder` now composes it with its schema-factory and external-import handling. The seam is the dependency list rather than a path string, so `indexFiles` branch selection cannot drift either. Export shaping stays caller-owned: it legitimately differs between callers. No behaviour change — core's own logic, relocated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`writeSpecs` built the schema→tag map for `schemas.splitByTags`. Extra files (`*.resource.ts` and friends) are rendered earlier, inside `getApiBuilder`, so generators that emit them had no way to route schema imports into the same tag subdirectories the writers would later use. Build the map in `getApiBuilder` instead, carry it on `WriteSpecBuilder`, and pass it to `ClientExtraFilesBuilder` as an optional fourth argument. `writeSpecs` now consumes it rather than recomputing it, so every consumer routes through one map instead of two derivations that can drift. The map is a required field, not optional: a construction site that omits it should fail to compile rather than silently degrade to a flat layout. `getApiBuilder` takes `componentSchemas` because the map must be built over the merged schema list. Component schemas are appended after it returns, so building from the operation-derived schemas alone yields a map missing nearly every schema — which collapses tag routing to flat without any error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`buildSchemaImportDependencies` re-derived core's schema-import rule instead of
sharing it, and had drifted from it on four points. Resource files are rendered
before any mode writer runs, so they cannot observe the writers' decisions —
re-deriving the rule is what let the two disagree.
Delegate to core's `resolveSchemaImportDependencies`. Four fixes follow:
- `schemas.importPath` is now honored. It was ignored, so a config setting
`schemas: { path, importPath: '@acme/models' }` emitted:
pets.service.ts import { Pet } from '@acme/models';
pets.resource.ts import { Pet } from '../../../../../../models/.../schemas';
The deep relative path crosses a package boundary, which module-boundary lint
rules reject (e.g. Nx's `@nx/enforce-module-boundaries`), so
`retrievalClient: 'both'` could not be used from a monorepo library. Package
specifiers are emitted verbatim, and carry no file extension under
`indexFiles: false` — `@acme/models/pet.js` would not resolve.
- `schemas.splitByTags` is now honored. With `indexFiles: false` the tag
subdirectory was omitted, so the resource file imported `<schemas>/pet` while
the schema is written to `<schemas>/pets/pet`.
- Zod schema filenames now derive from the TS identifier, matching how
`writeZodSchemas` names the files. Deriving from `schemaName` pointed at a
file that is never emitted whenever the two differ.
- Imports dedupe on name, alias, values and default rather than name alone, so
the same schema imported under two aliases no longer loses one.
Tested as one matrix asserted twice — against the shared helper in core, and
against rendered `*.resource.ts` output here — plus an end-to-end test that
generates both files and asserts they import the same symbol from the same
module.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughSchema import resolution moves into a shared core utility. Schema tag mappings now flow from API construction through writers and Angular resource generation. Tests cover package, relative, tag-split, extension, Zod, and deduplication cases. ChangesSchema import routing
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR aligns Angular resource-file schema imports and tag splitting with the configured output, preventing invalid monorepo import paths. No actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant importOpenApi
participant getApiBuilder
participant generateExtraFiles
participant AngularResource
participant resolveSchemaImportDependencies
importOpenApi->>getApiBuilder: pass componentSchemas
getApiBuilder->>generateExtraFiles: pass schemaTagMap
generateExtraFiles->>AngularResource: generate extra resource files
AngularResource->>resolveSchemaImportDependencies: resolve schema dependencies
resolveSchemaImportDependencies-->>AngularResource: return grouped imports
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Angular retrievalClient: 'both' resource-file generation so *.resource.ts resolves schema imports using the same logic as core writers, ensuring consistent imports (including honoring schemas.importPath and schemas.splitByTags) and avoiding cross-package deep relative paths in monorepos.
Changes:
- Extracts schema-import dependency resolution into a shared core utility (
resolveSchemaImportDependencies) and reuses it from both core writers and Angular*.resource.tsgeneration. - Computes and propagates a single schema→tag map during API building so extra files and mode writers route schema imports identically.
- Adds targeted unit + integration tests (core util matrix, Angular resource output matrix, and an end-to-end service/resource agreement test).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orval/src/write-specs.ts | Stops recomputing the schema→tag map in writeSpecs, instead using the one carried on the builder. |
| packages/orval/src/import-open-api.ts | Passes component-derived schemas into getApiBuilder so the schema→tag map is built over the merged schema list. |
| packages/orval/src/generate-spec.test.ts | Adds E2E coverage ensuring Angular service/resource files import the same schema from the same module. |
| packages/orval/src/client.ts | Threads schemaTagMap through the extra-files generation call chain. |
| packages/orval/src/api.ts | Builds schemaTagMap during API building (before extra files render) and passes it to extra-file generators. |
| packages/core/src/writers/generate-imports-for-builder.ts | Replaces duplicated schema-import path logic with the shared resolver util. |
| packages/core/src/utils/schema-import-path.ts | Introduces resolveSchemaImportDependencies (+ dedupeSchemaImports) as the shared single source of truth for schema import dependency resolution. |
| packages/core/src/utils/schema-import-path.test.ts | Adds a matrix test suite for schema-import dependency resolution behavior. |
| packages/core/src/utils/index.ts | Exports the new schema import resolver utility via the utils barrel. |
| packages/core/src/types.ts | Extends extra-file builder signature to accept schemaTagMap and adds schemaTagMap to builder types for propagation. |
| packages/core/src/test-utils/split-modes.ts | Updates test builder factory to include the new schemaTagMap field. |
| packages/angular/src/http-resource.ts | Routes resource-file schema imports through the shared core resolver; honors schemas.importPath and tag subdirectories. |
| packages/angular/src/http-resource.test.ts | Adds a mirrored matrix asserting *.resource.ts schema import resolution matches core behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@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: |
Contributor
Author
the-ult
marked this pull request as draft
August 5, 2026 13:26
Zod schema imports were built with a hardcoded `.zod` suffix and an extension taken from `output.fileExtension`. With a custom `fileExtension` the writers emitted `model/pet.gen.ts` while the imports pointed at `./model/pet.zod.gen`. The import tail now comes from `output.schemaFileExtension`, which is the option the writers use to name the files. Also addresses review feedback on the shared helper: - Route the remaining "is package import" derivation in `generateImportsForBuilder` through `getSchemasImportPath`. - Pass the prebuilt schema tag map to `writeSchemasTagsSplit` instead of rebuilding it there, so the map really is computed once. - Rename `getHttpResourceRelativeSchemasPath` to `getHttpResourceSchemasModule`, because it can return a package specifier. - Align the parameter order of `resolveSchemaImportDependencies` with the existing convention. - Make `schemaTagMap` optional at every declaration site. - Trim the JSDoc and comments to the essentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
buildSchemaImportDependencies deduped schema imports before forcing
every export to `{ values: true }` for Zod output. dedupeSchemaImports
keys on the `values` field, so a schema imported once as a value
(e.g. the auto-detected parse schema) and once as a type (e.g. a
named path-params type) survived the pre-force dedupe as two distinct
entries, which the force-to-values step then collapsed into identical
entries without deduping again.
Core's generateDependency already uniques named-import specifiers at
render time, so this never produced invalid duplicate specifiers in
generated files, but the intermediate export list buildSchemaImportDependencies
returns should be self-consistent regardless of that downstream
safety net.
Adds a regression test constructing the type-only + value duplicate
case.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
the-ult
marked this pull request as ready for review
August 14, 2026 22:04
…rce-schemas-import-path
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
Angular's
retrievalClient: 'both'emits a sibling*.resource.tsnext to each service. That file resolved schema imports with its own copy of core's rule, and the copy had drifted. Withschemas: { path, importPath: '@acme/models' }the two files disagreed:That relative path crosses a package boundary, so module-boundary lint rules (Nx's
@nx/enforce-module-boundaries, for example) reject it.retrievalClient: 'both'was unusable from a monorepo library.Why this fix
Extra files are rendered in
getApiBuilder, before any mode writer runs. A resource file cannot see the writers' import decisions, so it derives them fromoutput— and two derivations of one rule drift.The rule now lives once, in
resolveSchemaImportDependencies(core/src/utils/schema-import-path.ts). BothgenerateImportsForBuilderand Angular's resource builder call it.The schema→tag map moves to
getApiBuilderand rides onWriteSpecBuilder, sowriteSchemasTagsSplitno longer rebuilds it. It is built over the merged schema list, because component schemas are appended aftergetApiBuilderreturns.User-visible changes
schemas.importPathis honored in*.resource.ts— the reported bug. The specifier is emitted verbatim, with no file extension underindexFiles: false.schemas.splitByTagsis honored in*.resource.ts. The tag subdirectory was missing underindexFiles: false.output.schemaFileExtension, so they point at the file that is written. Before, a customfileExtensionwrotemodel/pet.gen.tsbut imported./model/pet.zod.gen.Tests
core/src/utils/schema-import-path.test.ts— the helper acrossindexFiles,splitByTags, zod/TypeScript,importPathset or unset, and a customfileExtension.angular/src/http-resource.test.ts— the same options against rendered*.resource.ts.orval/src/generate-spec.test.ts— end to end: the service file and the resource file must import the same symbol from the same module. A unit test on the resource builder alone cannot catch a disagreement, because it sees only one side.Known, not fixed
TypeScript schemas with a custom
fileExtensionandschemas.importPathandindexFiles: falsestill import@acme/models/petwhile the file ispet.gen.ts. Same family, outside this change.Verification
build:release,typecheck,lint,format:check,test,test:snapshots— all green. No committed fixture or sample output changed.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests