feat(core): add schemas.importPath for package import specifiers - #3548
Conversation
📝 WalkthroughWalkthroughAdds support for ChangesSchema importPath Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 configuring output.schemas.importPath so generated clients (and related helpers) can import schemas via a package specifier instead of computed relative filesystem paths.
Changes:
- Validates and preserves
schemas.importPathduring options normalization. - Updates core writers/import generation to prefer the configured package specifier and to omit NodeNext/Node16 file extensions for package imports.
- Adds documentation and broad test coverage for
schemas.importPathacross generation modes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orval/src/utils/options.ts | Validates schemas.importPath and passes it through normalization. |
| packages/orval/src/utils/options.test.ts | Adds normalization tests for schemas.importPath and invalid values. |
| packages/orval/src/generate-spec.test.ts | Adds generation tests asserting schema imports use the package specifier across modes. |
| packages/core/src/writers/tags-mode.ts | Prefers package schemas.importPath over relative path computation. |
| packages/core/src/writers/split-tags-mode.ts | Prefers package schemas.importPath over relative path computation in tags-split. |
| packages/core/src/writers/split-mode.ts | Prefers package schemas.importPath over relative path computation in split mode. |
| packages/core/src/writers/single-mode.ts | Prefers package schemas.importPath over relative path computation in single mode. |
| packages/core/src/writers/generate-imports-for-builder.ts | Omits local-file import extensions when importing from a package specifier. |
| packages/core/src/writers/generate-imports-for-builder.test.ts | Adds tests for package import behavior (indexFiles, NodeNext, zod suffix, schemaFactory). |
| packages/core/src/utils/schemas-options.ts | Introduces getSchemasImportPath helper to extract importPath from schemas config. |
| packages/core/src/utils/schemas-options.test.ts | Adds unit tests for getSchemasImportPath. |
| packages/core/src/utils/index.ts | Re-exports the new schemas options helper. |
| packages/core/src/utils/assertion.test.ts | Moves/duplicates a $dynamicRef assertion into the general isReference test. |
| packages/core/src/types.ts | Extends schema option types with optional importPath. |
| packages/core/src/generators/factory.ts | Changes factory/schema import path resolution to use schemas.importPath. |
| packages/core/src/generators/factory.test.ts | Adds tests for factory imports when schemas.importPath is provided. |
| docs/content/docs/reference/configuration/output.mdx | Documents object-form schemas and the new importPath option with requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…al-labs#3535) Allow generated client files to import schema types from a package specifier (e.g. '@acme/models') instead of computing a relative filesystem path. This unblocks use cases where the schemas and client outputs sit in separate TypeScript compilation roots or in secondary entrypoint packages. When `schemas.importPath` is set, all four write modes and the factory generators emit imports from the configured package specifier. The filesystem `path` is still used for the on-disk schema output, and validation rejects empty strings, relative paths, absolute paths, and whitespace-only values. - core: add `importPath` to `SchemaOptions` / `NormalizedSchemaOptions` - core: extract `getSchemasImportPath` helper into its own module - core: update all writers (single/split/tags/split-tags) to use the helper - core: update factory.ts to resolve factory and type imports from `importPath` - core: skip per-file extension in `generateImportsForBuilder` for package imports - orval: tighten `normalizeSchemasOption` validation for `importPath` - docs: document `schemas` object form and `importPath` requirements - tests: cover all four modes, zod suffix, faker subpath, and `outputDirectory` bypass Closes orval-labs#3535
78af92c to
7eff340
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/orval/src/generate-spec.test.ts (1)
882-883: ⚡ Quick winMake import assertion regex consistent and more precise.
The regex pattern
/from\s+'@acme\/models/lacks the closing quote, which could match false positives likefrom '@acme/models/subpath'. The test at line 1021 correctly includes the closing quote:/from\s+'@acme\/models'/. For consistency and precision, standardize all four assertions to include the closing quote.♻️ Proposed fix to add closing quotes
const content = await fs.readFile(targetFile, 'utf8'); - expect(content).toMatch(/from\s+'`@acme`\/models/); + expect(content).toMatch(/from\s+'`@acme`\/models'/); expect(content).not.toMatch(/from\s+'\.\./);Apply the same pattern at lines 920, 956, and 989.
Also applies to: 920-921, 956-957, 989-990
🤖 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/generate-spec.test.ts` around lines 882 - 883, Update the four import-match assertions that currently use the incomplete regex /from\s+'`@acme`\/models/ so they require the closing quote; change each to /from\s+'`@acme`\/models'/ to avoid matching subpaths (same pattern already used at the assertion around line 1021). Specifically update the occurrences that pair with expect(content).toMatch(...) at the positions shown in the diff so all four tests use the precise regex and keep the companion expect(content).not.toMatch(/from\s+'\.\./) assertions unchanged.
🤖 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/reference/configuration/output.mdx`:
- Around line 151-169: Update the requirements list for importPath to explicitly
enumerate the validation rules applied during config normalization: state that
importPath is rejected if empty or only whitespace (including
whitespace-padded), if it is a relative specifier (starts with ./ or ../), or if
it is an absolute path (starts with /), and mention that these checks occur
during config normalization that rejects invalid importPath values; ensure this
text is added near the existing "Requirements when using `importPath`" bullets
and references the `importPath` setting so users see the exact constraints.
In `@packages/orval/src/utils/options.ts`:
- Around line 139-143: The current validation in the if block that checks
schemas.importPath using startsWith('/') misses Windows absolute paths; update
the check for schemas.importPath in packages/orval/src/utils/options.ts (the
conditional that throws the Error for absolute paths) to also detect
Windows-style absolute paths (drive-letter like "C:\..." and UNC paths starting
with "\\" )—best done by using Node's path.isAbsolute(schemas.importPath) or
additional regex checks for /^[A-Za-z]:\\/ and /^\\"\\/" to ensure any absolute
path (POSIX or Windows) triggers the same Error message that currently runs for
POSIX absolute paths.
---
Nitpick comments:
In `@packages/orval/src/generate-spec.test.ts`:
- Around line 882-883: Update the four import-match assertions that currently
use the incomplete regex /from\s+'`@acme`\/models/ so they require the closing
quote; change each to /from\s+'`@acme`\/models'/ to avoid matching subpaths (same
pattern already used at the assertion around line 1021). Specifically update the
occurrences that pair with expect(content).toMatch(...) at the positions shown
in the diff so all four tests use the precise regex and keep the companion
expect(content).not.toMatch(/from\s+'\.\./) assertions unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fda4abaa-c573-4a89-b78d-f774048951a3
📒 Files selected for processing (17)
docs/content/docs/reference/configuration/output.mdxpackages/core/src/generators/factory.test.tspackages/core/src/generators/factory.tspackages/core/src/types.tspackages/core/src/utils/assertion.test.tspackages/core/src/utils/index.tspackages/core/src/utils/schemas-options.test.tspackages/core/src/utils/schemas-options.tspackages/core/src/writers/generate-imports-for-builder.test.tspackages/core/src/writers/generate-imports-for-builder.tspackages/core/src/writers/single-mode.tspackages/core/src/writers/split-mode.tspackages/core/src/writers/split-tags-mode.tspackages/core/src/writers/tags-mode.tspackages/orval/src/generate-spec.test.tspackages/orval/src/utils/options.test.tspackages/orval/src/utils/options.ts
|
@aqeelat did you review all AI comments. Feel free to comment and resolve them? |
not yet. I'll go over them now. |
…sion Address review feedback on orval-labs#3548: - docs: enumerate the validation rules applied to schemas.importPath during config normalization (empty, whitespace, relative, POSIX and Windows absolute paths). - core: reject Windows-style absolute paths in addition to POSIX ones (drive-letter like 'C:\...', UNC like '\\server\share'). - core: tighten import-match regexes in generate-spec.test.ts to require the closing quote so subpath imports don't satisfy the assertion. Tests cover the new Windows path rejections.
- Split whitespace validation into two distinct checks (whitespace-only vs. padded) with accurate error messages for each - Add test for padded whitespace rejection - In split mode, append .factory suffix when pkgBase is set so factory function and type imports resolve to distinct subpaths - Update factory tests to match corrected behavior - Add cross-reference from requirements list to validation section in docs
|
@melloware all done |
…on (#3658) * docs(output): restructure importPath validation and sync zh translation Nest the "Validation of importPath" section under ### importPath as an H4 subsection instead of a sibling H3 placed after splitByTags. The anchor #validation-of-importpath is preserved. Sync the Chinese translation (zh/reference/configuration/output.mdx) with content from #3548, #3595, #3613, and #3618 that was only added to the English docs: - Expand ## schemas with String/Object forms, property table, importPath subsection (incl. requirements + validation), and splitByTags subsection - Add schemasImportPath row to the Faker generator table and a dedicated #### schemasImportPath subsection - Add missing MSW and Faker generator table rows for full zh/en parity (delay, useExamples, generateEachHttpStatus, locale, etc.) * docs(zh): translate String/Object form and Validation headings to Chinese Translate descriptive headings introduced in the previous commit from English to Chinese, keeping config property name headings (importPath, splitByTags) in English per existing convention: - "String form" → "字符串形式" - "Object form" → "对象形式" - "Validation of importPath" → "importPath 校验" (link + heading updated)
Closes #3535
Fix #353
Problem
When
output.schemaspoints to a folder outside the generated client target, Orval emits imports based on filesystem paths. This works when schemas and clients are part of the same TypeScript compilation root, but can fail for package secondary entrypoints, where imports likeimport type { Pet } from '/libs/client/models'are emitted instead of a proper package specifier.Solution
Allow
output.schemasto specify an import specifier independent from the filesystem output path:When
importPathis set, generated client files import schema types from that package specifier. The filesystempathis still used for the on-disk schema output.Changes
importPathtoSchemaOptions/NormalizedSchemaOptionsgetSchemasImportPathhelper into its own moduleimportPath.js/.tsextension ingenerateImportsForBuilderfor package imports (no extension on package specifiers)normalizeSchemasOptionvalidation forimportPath— rejects empty strings, relative paths, absolute paths, and whitespace-only valuesschemasobject form, theimportPathoption, and the zod.zodsubpath requirementfactoryMethods.outputDirectorybypassNotes
importPathis set,factoryMethods.outputDirectoryis bypassed (factories resolve imports against the package specifier rather than the on-disk factory output directory) — documented.type: 'zod') the per-file suffix is.zod, so the package must expose./pet.zod(e.g.,@acme/models/pet.zod).Summary by CodeRabbit