feat(mock): emit strict Mock return types when required and nonNullable are both true#3529
Conversation
…lable are both true When override.mock.required and override.mock.nonNullable are both enabled, schema and operation mock factories now return a strict Mock alias while keeping Partial<T> overrides (including null). Also forwards nonNullable through getMockWithoutFunc for operation response mocks. Fixes orval-labs#3525 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds strict-mock detection and formatting helpers, uses them to compute and emit typed factory signatures for faker and MSW generators (including array-item and oneOf helpers), forwards ChangesStrict Mock Type System and Integration
Sequence Diagram(s) sequenceDiagram
participant CLI as Orval Generator
participant MockTypes as mock-types
participant Formatter as formatMockFactoryDeclaration
CLI->>MockTypes: getMockFactorySignatureParts(typeName, mockOptions, options)
MockTypes->>Formatter: param, returnType, returnCast
Formatter->>CLI: formatted export const ... declaration
CLI->>FileSystem: emit factory + strict helper blocks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Use generic MockWithNullableOverrides so getPetMock() returns a complete
PetMock, while getPetMock({ tag: null }) correctly types tag as string | null.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
![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
🤖 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/mock/src/faker/index.test.ts`:
- Line 200: The inline literal assigned to `context` is being cast directly to
`GeneratorOptions['context']` which triggers TS2352; change the cast to go
through `unknown` like the `baseOptions` pattern (i.e., cast `context` first to
`unknown` and then to `GeneratorOptions['context']`) so the type assertion is
accepted; update the cast at the `context` assignment where
`GeneratorOptions['context']` is used to mirror the `baseOptions` double-cast
approach.
In `@packages/mock/src/mock-types.test.ts`:
- Around line 13-87: Add a unit test for getStrictMockTypeDeclarations that
verifies it deduplicates and bulk-generates mock type aliases: call
getStrictMockTypeDeclarations with an array containing duplicate schema names
(e.g., 'Pet', 'Pet', 'Error[]') and assert the returned string contains exactly
one declaration per unique base name (e.g., the same text produced by
getStrictMockTypeDeclaration('Pet') and getStrictMockTypeDeclaration('Error')
and not duplicated) and that declarations are joined in the expected output
ordering/format.
- Around line 62-80: Add tests for getSchemaTypeNamesFromResponses that cover
the two defensive paths: one test where a response import uses an alias (set
import.alias to a different string and ensure the alias is returned instead of
import.name) and another test where a response has a falsy value (value:
undefined or empty string) to confirm it is skipped; reference the
ResReqTypesValue shape and the getSchemaTypeNamesFromResponses function when
adding these cases to mock-types.test.ts.
In `@packages/mock/src/msw/index.ts`:
- Around line 20-26: Remove the unused import getMockFactoryReturnType from the
import list in the MSW mock module; update the import statement that currently
includes getMockFactoryReturnType alongside applyStrictMockReturnType,
getSchemaTypeNamesFromResponses, getStrictMockTypeDeclarations, and isStrictMock
so it only imports the actually used symbols (e.g., keep
applyStrictMockReturnType and the others) to eliminate the unused reference.
🪄 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: a912e04b-2070-4510-a082-d8546940a90b
📒 Files selected for processing (20)
packages/mock/src/faker/getters/array-item-factory.tspackages/mock/src/faker/index.test.tspackages/mock/src/faker/index.tspackages/mock/src/faker/resolvers/value.tspackages/mock/src/mock-types.test.tspackages/mock/src/mock-types.tspackages/mock/src/msw/index.test.tspackages/mock/src/msw/index.tspackages/mock/src/msw/mocks.tstests/__snapshots__/mock/issue-3525-oas31/endpoints.tstests/__snapshots__/mock/issue-3525-oas31/model/index.faker.tstests/__snapshots__/mock/issue-3525-oas31/model/index.tstests/__snapshots__/mock/issue-3525-oas31/model/pet.tstests/__snapshots__/mock/issue-3525/endpoints.tstests/__snapshots__/mock/issue-3525/model/index.faker.tstests/__snapshots__/mock/issue-3525/model/index.tstests/__snapshots__/mock/issue-3525/model/pet.tstests/configs/mock.config.tstests/specifications/issue-3525-oas31.yamltests/specifications/issue-3525.yaml
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mock/src/faker/getters/array-item-factory.ts`:
- Around line 16-22: The import order violates simple-import-sort: place imports
from ../../mock-types before ../../types; specifically, reorder the import
statements so formatMockFactoryDeclaration and getMockFactorySignatureParts
(from '../../mock-types') come before the MockSchema type import (from
'../../types'), keeping the local imports overrideVarName and extractItemsRef
unchanged to satisfy the linter.
🪄 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: a098dcc4-7971-4130-9614-9867486f5bed
📒 Files selected for processing (12)
packages/mock/src/faker/getters/array-item-factory.tspackages/mock/src/faker/index.test.tspackages/mock/src/faker/index.tspackages/mock/src/faker/resolvers/value.tspackages/mock/src/mock-types.test.tspackages/mock/src/mock-types.tspackages/mock/src/msw/index.test.tspackages/mock/src/msw/index.tstests/__snapshots__/mock/issue-3525-oas31/endpoints.tstests/__snapshots__/mock/issue-3525-oas31/model/index.faker.tstests/__snapshots__/mock/issue-3525/endpoints.tstests/__snapshots__/mock/issue-3525/model/index.faker.ts
✅ Files skipped from review due to trivial changes (3)
- tests/snapshots/mock/issue-3525-oas31/model/index.faker.ts
- tests/snapshots/mock/issue-3525-oas31/endpoints.ts
- tests/snapshots/mock/issue-3525/endpoints.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mock/src/msw/index.test.ts
- packages/mock/src/faker/index.test.ts
- tests/snapshots/mock/issue-3525/model/index.faker.ts
Preserve semicolon and blank-line conventions for MSW response mocks and schema faker exports. Co-authored-by: Cursor <cursoragent@cursor.com>
Add M extends Record<keyof T, unknown> so generated strict mock helpers typecheck under TS2536. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu I'll have to ask for your review skills :) |
|
Thanks for the ping @Hypenate, and nice work extracting the strict-mock logic into a testable Repro: OAS 3.0, 1. Duplicate type declarations across operations → TS2300
Any spec where ≥2 operations share a file (single mode, or one tag file) trips this — e.g. Suggestion: emit the helpers and each 2. Schema-factory imports treated as type names → TS2749For the array endpoint the response delegates to the faker factory export type getPetMockMock = {
[K in keyof Required<getPetMock>]: NonNullable<Required<getPetMock>[K]>;
};
(The alias is also dead — the array factory's return type already resolves to Things that look good
One design questionThe issue listed per-call generic narrowing as out-of-scope ("TS will not narrow the return type when you pass Repro spec available if useful. Thanks again! |
…orts Dedupe strict mock helper types per endpoints file and ignore faker factory value imports. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu I gave it another go. |
|
in my opinion "shrinking the dedupe" surface is always ideal so I am ok with the narrowing if necessary! |
wadakatu
left a comment
wadakatu
left a comment
There was a problem hiding this comment.
Re-pulled 6b817246c — both original issues are fixed (multi-op + array specs generate and tsc clean, and the issue-3525-multi fixture is a great addition 👍).
Following up on the dedup approach before it lands. After this change the strict-mock types are emitted only by dedupeStrictMockTypeDeclarations re-deriving schema names via regex from the rendered output, and it runs on every mock file regardless of the strict flags. That string-scraping regresses today for any schema whose name ends in Mock — including users who don't touch the strict flags at all. I left inline notes on the exact spots; full repro below.
Repro — plain non-strict mock (no required/nonNullable), one schema named WidgetMock:
components:
schemas:
WidgetMock:
type: object
required: [id]
properties: { id: { type: integer }, label: { type: string, nullable: true } }endpoints.ts(8,3): error TS2440: Import declaration conflicts with local declaration of 'WidgetMock'.
endpoints.ts(75,24): error TS2304: Cannot find name 'Widget'.
The dedup injects strict helpers + export type WidgetMock = { [K in keyof Required<Widget>] … } into the non-strict file — it collides with the imported model (TS2440) and references a non-existent Widget captured from WidgetMock (TS2304). The same breakage happens in strict mode for a *Mock-named schema. Normal non-strict specs without a *Mock schema are unaffected — the dedup is a clean no-op there, so the blast radius is "any schema whose name ends in Mock".
Two changes would make it robust:
- Only run the hoist when strict mode is actually on (
isStrictMock(override.mock)), so non-strict output is never rewritten. - Build the header from the real strict type names you already compute (
getStrictMockTypeName/getSchemaTypeNamesFromResponses) instead of regex-scanning the rendered string — that also removes the whitespace coupling inSTRICT_MOCK_SCHEMA_DECL_PATTERN/replaceAll(helperBlock, …).
A fixture with a schema named WidgetMock (both strict and non-strict) would lock the regression down. Happy to help if useful — thanks again for iterating on this! 🙏
| return ( | ||
| implementation.includes('export type KeysWithNull') || | ||
| implementation.includes('MockWithNullableOverrides<') || | ||
| /\b[A-Z]\w*Mock\b/.test(implementation) |
There was a problem hiding this comment.
This /\b[A-Z]\w*Mock\b/ gate matches any PascalCase identifier ending in Mock, including a user schema literally named WidgetMock. Because dedupeStrictMockTypeDeclarations runs for every mock file (see finalizeMockImplementation wiring), this flips to true for non-strict output too and triggers the rewrite. Gating on the actual strict flag (isStrictMock(override.mock)) rather than a name heuristic would avoid the false positive.
| names.add(match[1]); | ||
| } | ||
|
|
||
| for (const match of implementation.matchAll(/\b([A-Z]\w*)Mock\b/g)) { |
There was a problem hiding this comment.
This captures Widget from the model type WidgetMock, so buildStrictMockTypeFileHeader then emits:
export type WidgetMock = {
[K in keyof Required<Widget>]: NonNullable<Required<Widget>[K]>;
};Widget doesn't exist (→ TS2304) and the alias name WidgetMock collides with the imported model (→ TS2440). Re-deriving schema names from the rendered string is the fragile root cause here — threading the real type names through as structured data (you already compute them via getStrictMockTypeName / getSchemaTypeNamesFromResponses) would remove this class of bug.
| export function dedupeStrictMockTypeDeclarations( | ||
| implementation: string, | ||
| ): string { | ||
| let body = implementation.replaceAll(INVALID_STRICT_MOCK_DECL_PATTERN, ''); |
There was a problem hiding this comment.
dedupeStrictMockTypeDeclarations runs unconditionally on every mock file (it's wired into finalizeMockImplementation plus both *Imports hooks), so non-strict generation gets rewritten too. An early isStrictMock(override.mock) guard — here or at the call sites — would scope this to the feature and remove most of the regression surface for non-strict users.
| footer: generateClientFooter, | ||
| imports: generateClientImports, | ||
| importsMock: generateMockImports, | ||
| finalizeMockImplementation: dedupeStrictMockTypeDeclarations, |
There was a problem hiding this comment.
This wires the hoist for all mock generation, regardless of the strict flags. As a result a non-strict user whose schema name ends in Mock gets strict helper types + a broken …Mock alias injected into their output (TS2440 / TS2304). Passing the strict flag (and ideally the structured strict type-name set) through here would let it stay a no-op for non-strict output.
Only hoist strict-mock helpers when required+nonNullable are set, pass schema names from generators instead of regex-scraping rendered output, and add WidgetMock fixtures to lock the non-strict regression. Co-authored-by: Cursor <cursoragent@cursor.com>
Explicitly annotate the normalized mockOutputs array so eslint no-unsafe-* passes in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadakatu I gave it another try, but maybe it's becoming too complex? |
|
First, the good news: I re-pulled On "is it too complex?" — honestly a little, but I don't think it's the amount of functionality; it's that the new structured-names path was added alongside the old regex-scraping path instead of replacing it. There are two parallel strategies right now:
So the complexity is basically the regex half, which only exists to clean up what the faker-schemas path emits inline. You can collapse it to one strategy: have That said, this simplification is an internal refactor — the PR as it stands fixes the regression and generates correct, compiling output (I verified locally). So there's no need to cram it all into this PR; I'd be totally fine doing the regex-layer removal / unification as a follow-up PR rather than piling more onto this one. Might be the easier path — what do you prefer? |
|
@wadakatu IF it's okay for @melloware to merge this, and then I'll do a follow-up PR to do the simplification, but atleast this PR stays under control. |
3 participants
Summary
override.mock.requiredandoverride.mock.nonNullableare both enabled, faker schema factories, operation response mocks, and MSWget*ResponseMockhelpers emit a strict{Schema}Mockbase type (Requiredkeys +NonNullableproperty types).null:getPetMock()→ completePetMock(tag: string)getPetMock({ tag: null })→tag: string | nullgetPetMock({ tag: 'abc' })→tag: stringoverrideResponsestays typed asPartial<T>(OpenAPI model) so all legitimate API-shaped overrides remain valid.getMockWithoutFuncnow forwardsnonNullableso operation/response mocks honor the flag at runtime.Generated signature (strict mode)
Test plan
mock-typeshelpers and strict mock generation infaker/index.test.tsandmsw/index.test.tsissue-3525,issue-3525-oas31)bun testinpackages/mockpasses (209 tests)Fixes #3525
Summary by CodeRabbit