Conversation
|
|
|
Reviewed the Task list (6/6 completed)
|
|
@malcolm-kee is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3681 +/- ##
==========================================
- Coverage 39.39% 38.30% -1.10%
==========================================
Files 520 536 +16
Lines 19279 19847 +568
Branches 5714 5879 +165
==========================================
+ Hits 7595 7602 +7
- Misses 9445 9885 +440
- Partials 2239 2360 +121
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
TL;DR — New Key changes
Summary ｜ 69 files ｜ 2 commits ｜ base: Plugin handler, processor, and walker pipeline
Per-type AST converters and property-name inference
Each module is focused:
Compound rules disambiguate by ancestor context — e.g.,
Circular reference detection and scoped depth tracking
At startup,
New
Faker v9/v10 compatibility layer
The
Operation request and response factories
Shared infrastructure and export pipeline
Documentation and test coverage
The test workspace
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid M1 implementation — the plugin follows established patterns well and the generated output looks clean. A few items to address, mostly around correctness of usesFaker propagation and a misleading cast.
packages/openapi-ts/src/plugins/@faker-js/faker/v1/toAst/enum.ts
Outdated
Show resolved
Hide resolved
| ctx: FakerWalkerContext, | ||
| schema: SchemaWithType<'enum'>, | ||
| ): Expression { | ||
| const members: Array<ReturnType<typeof $.literal | typeof $.fromValue>> = []; |
There was a problem hiding this comment.
The type annotation Array<ReturnType<typeof $.literal | typeof $.fromValue>> resolves to a union of their return types, which works. But ReturnType<typeof $.literal | typeof $.fromValue> is a somewhat unusual pattern — if both DSL helpers share a common base type you could simplify this. Low priority / style only.
|
@malcolm-kee I was going to ask about how you plan to handle locale and seed and I see they were removed 😬 the custom instance is a good pattern but I think we want to keep those options too – weaving the custom instance through every call will be a pain. Also, those options would basically enable static output: |
|
I understand about locale, but seed is kinda an oddball. Because if the user is using the global fakerjs instance, they can call However, it makes sense if we support a build time options, means we output the generated value instead of helper function, but, is there something we really want to do? |
|
I'll need to think more about |
malcolm-kee
force-pushed
the
feat/faker-plugin
branch
from
ef305ed to
54b3152
Compare
@hey-api/codegen-core
@hey-api/json-schema-ref-parser
@hey-api/nuxt
@hey-api/openapi-ts
@hey-api/shared
@hey-api/spec-types
@hey-api/types
@hey-api/vite-plugin
commit: |
|
Reviewed the faker plugin PR across all 58 changed files (3600+ additions). Submitted a comment-only review with 8 inline findings — mostly non-blocking robustness items around default bounds for unbounded constraints, a misleading type cast in enum handling, and a minor metadata inconsistency in operation response export. The plugin architecture faithfully follows the established pattern and the generated output is clean. Task list (7/7 completed)
|
dosubot
bot
added
size:XXL
|
@mrlubos I think it's at the right state for you to take a first look.
Docs is also added. |
There was a problem hiding this comment.
Well-structured plugin that faithfully follows the established pattern (config → handler → processor → walker → toAst → export). The architecture is clean, the generated output reads naturally, and the test coverage via milestone-staged specs is thorough. A few correctness and robustness items below — nothing blocking.
packages/openapi-ts/src/plugins/@faker-js/faker/v1/toAst/string.ts
Outdated
Show resolved
Hide resolved
packages/openapi-ts/src/plugins/@faker-js/faker/v1/toAst/enum.ts
Outdated
Show resolved
Hide resolved
|
|
||
| return { | ||
| applyModifiers(result) { | ||
| return result; |
There was a problem hiding this comment.
createVisitor is called once per process() invocation (inside createProcessor → process → processor.withContext). This means a new visitor + FakerWalkerContext is created for every schema definition. The EmitTracking object is shared across all invocations (passed from the handler), which is correct. But re-creating the visitor per schema means the fakerCtx.fakerAccessor DSL node is also recreated each time — verify this doesn't cause issues with reference identity if the DSL builder caches or interns expressions.
| processor: ProcessorResult; | ||
| schema: IR.SchemaObject; | ||
| statusCode: string; | ||
| tags?: ReadonlyArray<string>; |
There was a problem hiding this comment.
In exportOperationResponse, when the group is 'errors', role is set to group (i.e., 'errors') on line 94, but then role: 'response' is used on line 115 for registerSymbol. This means the processor receives role: 'errors' but the symbol metadata uses role: 'response'. The querySymbol API consumers always query with role: 'response' + statusCode, so this works for lookup — but the processor context having role: 'errors' while the symbol says role: 'response' is inconsistent. Consider aligning them.
| resource: meta.resource, | ||
| resourceId: meta.resourceId, | ||
| tool: 'typescript', | ||
| ...(meta.role ? { role: meta.role } : undefined), |
There was a problem hiding this comment.
The spread syntax ...(meta.role ? { role: meta.role } : undefined) produces ...undefined when meta.role is falsy. While this is valid JS (spreading undefined is a no-op), it's a pattern that some linters flag. A cleaner alternative:
...(meta.role && { role: meta.role }),|
|
||
| // Check if all items are "spreadable" (produce object-like values) | ||
| const allSpreadable = schemas.every( | ||
| (s) => s.$ref || s.type === 'object' || (s.items && s.logicalOperator), |
There was a problem hiding this comment.
The allSpreadable check includes s.items && s.logicalOperator as a spreadable condition — this catches union/intersection schemas that themselves contain items. But a union of primitives (e.g., oneOf: [string, number]) would have items and logicalOperator, pass this check, and be spread into an object literal, which wouldn't make sense. In practice this may not happen because allOf with primitive unions is unusual, but worth validating.
packages/openapi-ts-tests/faker/v1/__snapshots__/3.1.x/faker-m5/@faker-js/faker.gen.ts
Outdated
Show resolved
Hide resolved
| .$if( | ||
| final.usesAccessor, | ||
| (f) => { | ||
| const fakerPackagePath = plugin.config.locale | ||
| ? `@faker-js/faker/locale/${plugin.config.locale}` | ||
| : '@faker-js/faker'; | ||
| const fakerSymbol = plugin.external(`${fakerPackagePath}.faker`); | ||
| const fDecl = $.const('f').assign( | ||
| $.binary($('options').attr('faker').optional(), '??', $(fakerSymbol)), | ||
| ); | ||
| return f.do(fDecl, $.return(final.expression)); | ||
| }, | ||
| (f) => f.do($.return(final.expression)), | ||
| ); |
There was a problem hiding this comment.
This 14-line .$if(usesAccessor, ...) block is duplicated verbatim in operation.ts at lines 146–159 and 238–251. Consider extracting a shared helper, e.g.:
function buildAccessorBody(
plugin: FakerJsFakerPlugin['Instance'],
result: FakerResult,
): (f: FuncBuilder) => FuncBuilder {
// ...
}This would reduce each call site to a single .$if(result.usesAccessor, buildAccessorBody(plugin, result), ...).
|
I'm working on handling circular reference, which would becomes a problem when |
|
@malcolm-kee This is solved in validators, you can look there for inspiration and/or improve those too if you find a cleaner approach |
For this plugin it's more tricky than the validator. Zod/validbot needs to handle circular reference because you can't declare a schema that depends on itself, so on circular reference, the declaration syntax switch to For this plugin, the circular dependency issue is causing the faker function calling itself, causing call stack overflow. So we have to keep track of call stack depth, and then opt out when the call stack is too deep. The circular dependency is computed upfront from the graph in the context. And then during the walk, we check if $ref is circular - only then those factories get the |
|
Acknowledged the author's feedback on the circular-ref depth guard comment — the current nullable-only guard is correct since fully-required circular schemas are unfulfillable by definition. No code changes needed. Task list (1/2 completed)
|
|
Confirmed the author's fix for the Task list (2/3 completed)
|
|
No action needed — the single review thread was already addressed by the PR author and resolved. The |
|
Generate request data too. I think there is values to it - people might want to use them to test their server. |
packages/openapi-ts/src/plugins/@faker-js/faker/shared/operation-request.ts
Show resolved
Hide resolved
|
Re-reviewed the new commit (40b741f → 4b375b7). The change wraps request factory return types with |
|
@mrlubos faker 10 needs certain nodejs version (>22.13+) - I assume we had those different node build for compatibility check, so what should we do in this case? In there any library that we support encounters something like this before? |
|
I've installed faker v9 instead for our test setup. There is no breaking change at all and we're not using any faker v10 only feature, so let's not sweat it. |
|
Re-reviewed the new commits ( |
|
@malcolm-kee I'll probably bump to Node 22+. Node 20 is near its EOL date now |
malcolm-kee
force-pushed
the
feat/faker-plugin
branch
from
d8842ce to
248e883
Compare
There was a problem hiding this comment.
No new issues found. This is a substantial rewrite that squashes prior commits into a single clean implementation and addresses all previous review feedback.
Changes since last review
TL;DR: The PR was force-pushed as a single squashed commit that consolidates the v1/v10 split into just v10, adds operation request factories (M5), and addresses all prior review comments.
| Area | Change |
|---|---|
v1/ directory |
Removed — all codegen consolidated into v10/ |
operation-request.ts |
New: generates per-operation request factories with Omit<Data, 'url'> return types |
config.ts / types.ts |
Added compatibilityVersion, maxCallDepth, requests, responses config options |
enum.ts |
Cast fixed: as string → as string | number |
walker.ts |
LITERAL_FALSE → FALLBACK_UNDEFINED; circular ref gated on isCircularSchema; tuple now propagates hasCircularRef |
object.ts |
Now propagates usesAccessor and hasCircularRef from children |
specs/ |
New circular.yaml for 2.0.x, 3.0.x, 3.1.x |
| Zod snapshots | Updated from the new circular specs |
| Docs | Comprehensive faker.md documentation |
Prior review feedback
-
as stringcast in enum (reviews 1 & 2) -
LITERAL_FALSEnaming (review 1) - Circular ref gated on
isCircularSchema(review 3) - Tuple
hasCircularRefpropagation (review 3) -
Omit<Data, 'url'>for request factories (review 4) - Object
usesAccessorpropagation from children (review 1)
There was a problem hiding this comment.
Important
The 'id' suffix rule is very short and will produce false positives for common English words used as string property names.
Changes since last review
TL;DR: Adds suffix-based name matching as a fallback after exact/compound rules, plus a homePhone test property to validate it.
| Area | Change |
|---|---|
nameRules.ts |
New STRING_SUFFIX_RULES array + suffix fallback in stringNameToExpression |
specs/*.yaml |
Added homePhone: string property to Person schema across 2.0.x, 3.0.x, 3.1.x |
| Snapshots | Updated to reflect homePhone → f.phone.number() via suffix matching |
No prior review comments to address — previous review found no issues.
| // Order matters: first match wins. Suffixes are matched against normalized names. | ||
| // --------------------------------------------------------------------------- | ||
| const STRING_SUFFIX_RULES: Array<{ rule: NameRule; suffix: string }> = [ | ||
| { rule: { fakerPath: ['string', 'uuid'] }, suffix: 'id' }, |
There was a problem hiding this comment.
The suffix 'id' is only two characters. After normalization (valid → valid, paid → paid, grid → grid, avoid → avoid), any string property whose name ends in id would be mapped to string.uuid(). Consider using a longer suffix like 'Id'-aware matching (e.g. requiring a word boundary before id) or at minimum a 3+ character suffix such as uid or _id. Alternatively, a regex like /(?:^|[a-z])id$/ on the raw (un-normalized) name could catch userId, order_id, orderId without matching valid or paid.
3 participants
resolves #1485
What
A new faker plugin for openapi-ts that generates TypeScript factory functions using
@faker-js/faker. Given an OpenAPI schema, it produces typedfake*()functions that return realistic mock data matching the schema shape.Design
Architecture
Follows the established plugin pattern (same as valibot/zod):
handler -> processor -> walker -> toAst -> exportAst. The handler registers afakersymbol, iterates IR schemas viaplugin.forEach(), and delegates to a schema walker that dispatches each type to a converter function.Key differences from validator plugins
faker.number.int()) rather than pipe chains.(options?) => expr, not a const assignment to a schema object.optionsparameter. Only emitted when the factory body uses the faker instance. Literal factories (null,undefined) omit it to stay typecheck-clean.@hey-api/typescriptis active, factories get explicit return types (: Foo) by querying typescript plugin symbols. Absent gracefully when typescript plugin isn't configured.Factory signature
Optionstype withfaker(DI),includeOptional(control optional property inclusion),useDefault(use schema defaults vs faker-generated values) -- all supportboolean | numberfor probabilistic controlseedconfig -- the user can callfaker.seed()right before calling the factory function, so there is no need to expose an optionWhat's implemented
Core pipeline
All IR schema types handled:
string,number,integer,boolean,null,unknown,void,never,undefined,const,enum(including null members),object,array,tuple,$ref.Composition types
oneOf/anyOfunions with proper randomization:faker.helpers.arrayElement([() => variant1(), () => variant2()])()allOfintersections merge object properties from all sub-schemas via spread:{ ...fakeBase(options), ...fakeExtension(options) }as constto preserve literal types)nullgeneratefaker.datatype.boolean() ? <value> : null_callDepthparameter) to prevent infinite loops, configurable viamaxCallDepthoption (default10)Constraints & formats
email,date-time,date,uuid,uri/url,ipv4,ipv6-- mapped to correspondingfaker.internet.*/faker.string.*/faker.date.*callsfaker.helpers.fromRegExp(pattern)minLength/maxLength->faker.string.alpha({ length: { min, max } })minimum/maximum/exclusiveMinimum/exclusiveMaximum-> passed tofaker.number.int/float()(exclusive bounds adjusted +/-1 for integers)minItems/maxItems-> controlscountinfaker.helpers.multiple()Property-name-based faker inference
Infer semantically appropriate faker helpers from property names and ancestor context, producing more realistic mock data without requiring explicit schema formats.
email->faker.internet.email(),firstName->faker.person.firstName(),city->faker.location.city(),id->faker.string.uuid(), etc.age->faker.number.int({ min: 1, max: 120 }),price->faker.number.float({ min: 0, max: 10000 }),port->faker.internet.port()nameare disambiguated by parent schema --User.name->faker.person.fullName(),Company.name->faker.company.name()agewithminimum: 18produces{ min: 18, max: 120 }Optional properties & defaults
includeOptionaloption (supportsboolean | numberfor probabilistic inclusion)useDefaultoption controls whether to emit schema defaults or faker-generated values (supportsboolean | number)resolveCondition,MAX_CALL_DEPTH) are emitted lazily -- only when the generated code actually needs themPer-operation response factories
Generate faker factories for operation responses (e.g.
fakePostFooResponse), enabling mock tools like MSW to produce realistic response data per endpoint.fakeCreatePetResponse)fakeGetPetResponse200,fakeGetPetResponse404)fakeCreateJobResponse2Xx)GetPetResponses[200],GetPetErrors[404])Per-operation request factories
Generate faker factories for operation requests (e.g.
fakeUpdatePetRequest), combining body, path, query, and header parameters into one factory.Omit<OperationData, 'url'>Cross-plugin API
Two consumption patterns for other plugins:
Existing factories (for MSW) -- consumers use
querySymboldirectly with consistent metadata. All operation response factories register withrole: 'response'andstatusCode, so consumers always query the same way:Arbitrary schemas (for future plugins) -- Api methods following valibot's per-call plugin passing pattern:
toNode({ plugin, schema })-- inline faker expression via walkertoNodeRef({ plugin, schema })--referenceSymbollookup for$refschemas, falls back totoNodeConfiguration
locale-- generated code imports from@faker-js/faker/locale/<locale>instead of the defaultmaxCallDepth-- max recursion depth for circular schemas (default10)definitions/requests/responses-- individually toggle and customize naming/casing for each factory categorycompatibilityVersion-- target faker v9 or v10 (auto-detected from package.json)Possible improvements
User resolvers
Resolver types are defined in
resolvers/types.tsbut not wired into config or walker. Would enable user customization of faker output per schema type (string, number, array, object) via callback hooks.