Skip to content

feat: standalone module functions#3748

Open
ST-DDT wants to merge 31 commits intonextfrom
feat/standalone-module-functions
Open

feat: standalone module functions#3748
ST-DDT wants to merge 31 commits intonextfrom
feat/standalone-module-functions

Conversation

@ST-DDT
Copy link
Copy Markdown
Member

@ST-DDT ST-DDT commented Feb 28, 2026

Implements #2667


The goal of this PR is to convert our class/module based system to one, that uses standalone module functions and use them as the implementation of modules.

Non-goals of this PR:

  • create or expose nano bound module functions e.g. enFirstName (except for test purposes).

This PR will be created/achieve in the following steps:

  • Create FakerCore
  • Create transformation script (deleted before merge)
  • Apply transformation script (for early preview of code)
  • Fixup references to constants/types/enums, that would be to hard to implement via script
  • Run tree-shake tests faker.person.firstName() vs firstName(englishCore) vs firstName(nanoCore)
  • Create generate-module-tree script, that rebuilds the modules using the standalone functions
  • Update test suite, to use the standalone module function instead?
  • Update docs generation script to handle the new structure
  • Add faker vs standalone toggle to docs page for usage examples?

How to review:

  • Expect force pushes during the early stages of this PR (to keep the generated parts separate from manually written code/changes)
  • Review what the transform script does, not how it is implemented, as it gets deleted later on.
  • Revert the transform commit, apply the script and compare for deltas.
  • Check the transformed methods for issues (e.g. name conflict handling const vs imports)
  • Check the manual fixup commit for where stuff gets/should be moved
  • Run tests on your machine

Partial alternatives

@ST-DDT ST-DDT self-assigned this Feb 28, 2026
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent labels Feb 28, 2026
@netlify
Copy link
Copy Markdown

netlify bot commented Feb 28, 2026

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 8fa7e27
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/69b85d1c59bfae0008645eaa
😎 Deploy Preview https://deploy-preview-3748.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Feb 28, 2026

How should we distinguish between module functions and helper code for modules?

e.g. https://github.com/faker-js/faker/blob/next/src/modules/finance/bitcoin.ts

I considered prefixing the helper files with _.

So that:

  • src/modules/*/index.ts -> module index
  • src/modules/*/[a-z-]+.ts -> api
  • src/modules/*/_[a-z-]+.ts -> helpers

As an alternative, we could add an @api tag to the jsdocs.

What do you think?

@ST-DDT ST-DDT changed the title Standalone module functions feat: standalone module functions Feb 28, 2026
@ST-DDT ST-DDT force-pushed the feat/standalone-module-functions branch from 63d3440 to b1c0f00 Compare March 3, 2026 22:07
@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 5, 2026

Which variant do you prefer?

A)

resolveLocaleData(fakerCore, 'commerce', 'product_name')

or B)

assertLocaleData(fakerCore.definitions.commerce?.product_name, 'commerce.product_name')

A is shorter, but you can click on B to navigate to the source definitions. Also auto completion works better with B.

@xDivisionByZerox
Copy link
Copy Markdown
Member

Which variant do you prefer?

A)

resolveLocaleData(fakerCore, 'commerce', 'product_name')

or B)

assertLocaleData(fakerCore.definitions.commerce?.product_name, 'commerce.product_name')

A is shorter, but you can click on B to navigate to the source definitions. Also auto completion works better with B.

I need some context here:

  • What would be the signature of both functions?
  • Why is the string required in version B?
  • Are both functions able to resolve entries that are nested more deeply than the standard module/entry way? (think person.firstname.female)

Also I have the strong feeling that the standalone module function PRs would get more traction from the core team if they could be done in smaller chunks. Like maybe start with one module and let everyone experience of it works. Since modules have to stay anyway, there would be no harm in simply adding one module at the time. We could even release them as "developer preview" to gather intel on how they "feel" in every day coding.

Also, I'm aware that the "transform-once" script exists, but:

  • its currently not reliable (see failing tests)
  • its hard to understand what is actually happening
  • I'll check the generated files anyway to check if they align with the expected code style

I'm not saying it doesn't work! Throwing it at the entire code base at once might be a bit overwhelming I'd say.
WE did the same thing for the locale data normalization for example.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 98.82075% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.03%. Comparing base (e02536e) to head (8fa7e27).
⚠️ Report is 26 commits behind head on next.

Files with missing lines Patch % Lines
src/modules/helpers/from-reg-exp.ts 94.21% 5 Missing and 2 partials ⚠️
src/modules/helpers/replace-credit-card-symbols.ts 88.00% 6 Missing ⚠️
src/faker-core.ts 75.00% 1 Missing and 1 partial ⚠️
src/modules/person/_select-definition.ts 84.61% 0 Missing and 2 partials ⚠️
src/modules/commerce/isbn.ts 96.29% 1 Missing ⚠️
src/modules/commerce/upc.ts 95.45% 1 Missing ⚠️
src/modules/helpers/weighted-array-element.ts 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3748      +/-   ##
==========================================
+ Coverage   98.88%   99.03%   +0.14%     
==========================================
  Files         886     1182     +296     
  Lines        3062     3724     +662     
  Branches      556      672     +116     
==========================================
+ Hits         3028     3688     +660     
- Misses         30       31       +1     
- Partials        4        5       +1     
Files with missing lines Coverage Δ
src/internal/faker-to-core.ts 100.00% <100.00%> (ø)
src/internal/locale-proxy.ts 100.00% <100.00%> (ø)
src/module-registry.ts 100.00% <100.00%> (ø)
src/modules/airline/aircraft-type.ts 100.00% <100.00%> (ø)
src/modules/airline/airline.ts 100.00% <100.00%> (ø)
src/modules/airline/airplane.ts 100.00% <100.00%> (ø)
src/modules/airline/airport.ts 100.00% <100.00%> (ø)
src/modules/airline/flight-number.ts 100.00% <100.00%> (ø)
src/modules/airline/index.ts 100.00% <100.00%> (ø)
src/modules/airline/record-locator.ts 100.00% <100.00%> (ø)
... and 276 more

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 6, 2026

I need some context here:

/**
* Checks that the value is not null or undefined and throws an error if it is.
*
* @param value The value to check.
* @param path The path to the locale data.
*/
export function assertLocaleData<T>(

/**
* Resolves the locale data for the given category and entry.
*
* @template TCategory The category of the locale data to resolve.
* @template TEntry The entry of the locale data to resolve.
*
* @param fakerCore The FakerCore instance to get the locale data from.
* @param category The category of the locale data to resolve.
* @param entry The entry of the locale data to resolve.
*
* @returns The resolved locale data for the given category and entry.
*
* @throws {FakerError} If the category or entry is not defined in the locale data.
*
* @example
* arrayElements(fakerCore, resolveLocaleData(fakerCore, 'date', 'weekday')); // 'Sunday'
*
* @since 10.4.0
*/
export function resolveLocaleData<

It needs to the string to include it in the actual error message.

Are both functions able to resolve entries that are nested more deeply than the standard module/entry way? (think person.firstname.female)

Theoretically yes, but that's not what they are for.
The goal here is to extract the entries from the locale definitions and then pass them to the next process step usually arrayElement. Currently, all our locale data are structured that way.
Please note that definitions.person.first_name is the part that may be absent, everything below that we expect to follow the given type spec. We do not check whether only female is set and male is absent, we have a PersonEntryDefinition that we expect every locale to follow on that level.
If we users want to model their custom code differently, then they can do that either way.

The longer I think about this, the more I prefer B.


Also, I'm aware that the "transform-once" script exists, but its currently not reliable (see failing tests)

It was never meant to do all the work.

Also I have the strong feeling that the standalone module function PRs would get more traction from the core team if they could be done in smaller chunks. Like maybe start with one module and let everyone experience of it works.

#2667 (comment)
I would really love to see a working implementation that results in <<< 600 KiB like you proposed here, before actually rewriting the whole src code base.

This is kind of chicken and egg problem.
In order for showing that it works, I would like to show a real example e.g. the person module.
To implement that, I need the helpers module, that requires the number module, which requires some utils and the actual core.

Sounds simple, right?

The helper module contains fake. In the current implementation I mostly ignored how fake "knows" which methods exists.
We might have to add a module registry or something similar in the future.

Then there is this tiny detail called jsdocs verification tests, that I need to adjust as well.
I was able to dodge most of the api docs generation stuff for now.

Then there are naming conflicts with module methods e.g. lorem.word and word.word.

Should the module classes already start using the standalone methods?
This will only work if standalone fake works.

TLDR: Consider this a POC. I transformed the entire code base, to see which problems we need to handle.
This produces a working code base, that you can build youself locally and run any tests you would like to run with it.

For an in depth review, I can split this PR later into one per module, but we likely cannot do any releases once the first one lands until all of them are in (unless you don't make them public api).

FYI: Existing tests are passing now, but we need to consider how we change/duplicate them, to ensure both the standalone and the faker tree based ones have a desent coverage.

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 6, 2026

I did some tests with the current state of this PR to determine whether it results in smaller bundle sizes:

Just add the following exports to the faker's index.ts:

// Note this won't be the final export names
export { default as enPerson } from './locales/en/person';
export { avatar } from './modules/image/avatar';
export { urlLoremFlickr } from './modules/image/url-lorem-flickr';
export { firstName } from './modules/person/first-name';
export { lastName } from './modules/person/last-name';

Then I used the vue/vite-esm playground to test the bundle size.

FTF

<script setup lang="ts">
import { Faker, enPerson, FakerError } from "@faker-js/faker";
import { ref } from "vue";

const faker = new Faker({
  locale: { person: enPerson },
});

const fullName = ref(`${faker.person.firstName()} ${faker.person.lastName()}`);
const avatarUrl = ref(faker.image.avatar());
const natureImageUrl = ref(
  faker.image.urlLoremFlickr({ category: "nature" }) + new FakerError("test")
);
</script>

dist/assets/index-mO_qTbs-.js 182.21 kB │ gzip: 61.69 kB

grafik

SMF

<script setup lang="ts">
import { firstName, lastName, avatar, urlLoremFlickr, enPerson, FakerError, createFakerCore } from "@faker-js/faker";
import { ref } from "vue";

const fakerCore = createFakerCore({
  definitions: { person: enPerson },
});

const fullName = ref(`${firstName(fakerCore)} ${lastName(fakerCore)}`);
const avatarUrl = ref(avatar(fakerCore));
const natureImageUrl = ref(
  urlLoremFlickr(fakerCore, { category: "nature" }) + new FakerError("test")
);
</script>

dist/assets/index-GwB6agm0.js 136.59 kB │ gzip: 50.37 kB

grafik

Hard-Coded

<script setup lang="ts">
import { ref } from "vue";

const fullName = ref(`Firstname LastName`);
const avatarUrl = ref("https://cdn.jsdelivr.net/gh/faker-js/assets-person-portrait/female/512/75.jpg");
const natureImageUrl = ref(
  "https://loremflickr.com/2185/2550/nature?lock=7872953778781783Error:%20test"
);
</script>

dist/assets/index-DGlXtW4P.js 60.04 kB │ gzip: 24.07 kB

Result

The standalone module functions (SMF) take less space in the final bundle (-46 kB | gzip: -11 kB).
If you ignore vue and the other stuff, then its 30% smaller.
(And for whatever reason, we still ship extra parts of faker, maybe since definening a module class is a side effect?)

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 7, 2026

Interesting finding with createFakerCore:

return {
    definitions: Array.isArray(definitions)
      ? mergeLocales(definitions)
      : (definitions ?? {}),
    randomizer: randomizer ?? ({} as Randomizer),
    config: config ?? {},
  }

-> Some side effect, causes simpleFaker to be in the final bundle.

vs

return {
    definitions: Array.isArray(definitions)
      ? definitions[0]
      : (definitions ?? {}),
    randomizer: randomizer ?? ({} as Randomizer),
    config: config ?? {},
  }

-> Not included

mergeLocales doesn't even use/reference simpleFaker! (Same with generateMersenne53Randomizer)

🤔 WHY!?

@xDivisionByZerox
Copy link
Copy Markdown
Member

This is kind of chicken and egg problem.
In order for showing that it works, I would like to show a real example e.g. the person module.
To implement that, I need the helpers module, that requires the number module, which requires some utils and the actual core.

Sounds simple, right?

The helper module contains fake. In the current implementation I mostly ignored how fake "knows" which methods exists.
We might have to add a module registry or something similar in the future.

Then there is this tiny detail called jsdocs verification tests, that I need to adjust as well.
I was able to dodge most of the api docs generation stuff for now.

Then there are naming conflicts with module methods e.g. lorem.word and word.word.

Should the module classes already start using the standalone methods?
This will only work if standalone fake works.

TLDR: Consider this a POC. I transformed the entire code base, to see which problems we need to handle.
This produces a working code base, that you can build youself locally and run any tests you would like to run with it.

For an in depth review, I can split this PR later into one per module, but we likely cannot do any releases once the first one lands until all of them are in (unless you don't make them public api).

Thanks for the in depth explanation. I was already aware about the "person => helper => number" situation. That why I honestly had the number module in mind for this experiment. I was not aware of the faker.helper.fake situation.

@ST-DDT ST-DDT mentioned this pull request Mar 7, 2026
@ST-DDT ST-DDT force-pushed the feat/standalone-module-functions branch 2 times, most recently from d802170 to 6fbd6d0 Compare March 8, 2026 20:45
@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 8, 2026

I switched from resolveLocaleData to assertLocaleData.

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 16, 2026

I just noticed that my current implementation of a moduleRegistry for fake leads to a circular dependency.
The fake method references the global registry, , the global registry loads the methods via the module indexes, the helperEntries are defined in the index, but since the class is already initializing the helpers registry entry gets set to undefined and fake cannot call any helper methods directly. And if you execute the requests in a different order, the other modules are missing. I'm not exatly sure about my explanation, but I receive undefined modules in the registry.
I have to think about how to avoid that, maybe I will create a "registry.ts" file per module...

Depending on your point of view all methods that use fake experience a breaking change.

For now, I call all internal fake methods in the least breaking way, while still allowing for some level of tree shaking. Due to circular dependencies it is probably impossible to implememt in an entirely non-breaking way.

We might be able to relieve some of the pain points by switching from fake patterns to resolver functions internally. But that would be a breaking change and a separate PR for sure.

See also

I'll push the latest version once I get the fake issue resolved.

@ST-DDT ST-DDT force-pushed the feat/standalone-module-functions branch from 72e1d00 to c3b50eb Compare March 16, 2026 16:00
@ST-DDT ST-DDT force-pushed the feat/standalone-module-functions branch from c3b50eb to 61b4844 Compare March 16, 2026 17:32
@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 16, 2026

It's working...

@Shinigami92
Copy link
Copy Markdown
Member

It's working...

fantastic 🚀

is it possible to clean this PR up and/or spliterate it into smaller PR-chunks? e.g. extract preparing chores which does not affect directly the implementation of this PR, but is required for it

e.g. I see some utilities like casing-utils (which I usually do by https://www.npmjs.com/package/change-case), also some file namings now with a prefixed underscore

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 16, 2026

As for cleaning this PR, not sure how to do that (except for squashing a few commits here and there).


I would like to split this PR into smaller pieces, but not sure whether some commits needs to be kept together.

e.g.

  • basics/utils
  • scripts
  • helpers, int, string
  • other modules (each)
  • generate-module-tree + fixes

Before we start merging a bunch of stuff, there are a few things that we need to consider:

  • Do we consider the changes to fake "breaking"?
  • How do we export the actual SMFs?
  • Do we export the module registries?

@Shinigami92
Copy link
Copy Markdown
Member

PR #3748 Review: Standalone Module Functions

Reviewed by @Shinigami92 / Claude Max with /effort high | 2026-03-22


Summary

This PR introduces a fundamental architectural shift from class-based module methods to standalone functions, enabling tree-shaking and smaller bundle sizes. The approach is sound - the ~30% reduction in bundle size for targeted imports is compelling. I've done a deep review of the core infrastructure, the transformation pattern, and the resulting code. Below are my findings, concerns, and suggestions.


Architecture & Design

The overall architecture is clean and well-thought-out:

FakerCore (data container) -> Standalone Functions -> Module Classes (thin wrappers)

The FakerCore interface is minimal and elegant - just definitions, randomizer, and config. The separation of the data container from the class hierarchy is the right move for tree-shaking.

I especially like how the per-function mini-registries for fake() calls solve the circular dependency problem (e.g., streetAddress only importing { street, buildingNumber, secondaryAddress } rather than the full locationModule). This is the kind of design that makes tree-shaking actually work.


Critical Issues

1. fake() entrypoints divergence - Behavioral difference between standalone and class API

This is the most significant concern in the PR.

Class-based fake() (src/modules/helpers/index.ts:734-737):

entrypoints: ReadonlyArray<unknown> = [
  moduleRegistry,
  this.faker.rawDefinitions,
]

Standalone fake() (src/modules/helpers/fake.ts:181):

entrypoints: ReadonlyArray<unknown> = [fakerCore.definitions]

The class version resolves {{person.firstName}} through moduleRegistry.person.firstName (a function), while the standalone version would try to resolve it as definitions.person.firstName (which is first_name in locale data, not firstName - so it would fail or behave differently).

This means:

  • faker.helpers.fake('{{person.firstName}}') works (resolves via moduleRegistry)
  • fake(fakerCore, '{{person.firstName}}') would not work with the default entrypoints

Users migrating from class API to standalone would hit this silently. The JSDoc for standalone fake() says "Defaults to [ fakerCore.definitions ]" but the class version docs say "Defaults to the moduleRegistry and locale data." - this should be made more prominent as a key difference, or ideally the standalone version should also default to include the moduleRegistry (accepting the tree-shaking tradeoff for the fake() function specifically).

Suggestion: Consider making the standalone fake() default entrypoints match the class version, or at least add a very prominent warning in the JSDoc. Since fake() is inherently dynamic and can't be tree-shaken anyway, including moduleRegistry in its defaults is the pragmatic choice.

2. fakerToCore uses rawDefinitions - bypasses locale proxy validation

In src/internal/faker-to-core.ts:14:

definitions: faker instanceof Faker ? faker.rawDefinitions : {},

The Faker class wraps definitions in a LocaleProxy (createLocaleProxy(this.rawDefinitions)) that provides helpful error messages with documentation links when locale data is missing. By using rawDefinitions, standalone functions lose this DX benefit.

The standalone functions do use assertLocaleData() which throws on missing data, but the proxy provides additional niceties (prevents accidental writes, gives contextual error messages for missing categories).

Suggestion: Consider whether fakerToCore should use faker.definitions (the proxy-wrapped version) instead. This would preserve the error reporting quality users are used to. If not, document this as a known DX trade-off.

3. Missing public API exports

src/index.ts exports moduleRegistry but does NOT export:

  • createFakerCore
  • FakerCore (type)
  • FakerConfig (type)
  • fakerToCore
  • setDefaultRefDate / getDefaultRefDate

Without these, users cannot actually use standalone functions in a meaningful way. I understand this is a POC, but for reviewability it would be helpful to see the intended public API surface. Even if unexported for now, a comment or TODO noting the planned exports would help reviewers reason about the final shape.

Suggestion: At minimum, add TODO comments in src/index.ts indicating which new symbols are planned for export.

4. @ts-expect-error for private field access in fakerToCore

src/internal/faker-to-core.ts:15-19:

// @ts-expect-error: access private member field
randomizer: faker._randomizer,
config: {
  // @ts-expect-error: access private member field
  defaultRefDate: faker._defaultRefDate,
},

This creates a fragile coupling - any rename of these private fields would silently break at runtime (TypeScript would flag the @ts-expect-error as unnecessary, but only if someone notices).

Suggestion: Consider adding a toCore() method to SimpleFaker/Faker instead, which can access its own private fields safely. Or make _randomizer and _defaultRefDate protected/internal rather than private.


Moderate Concerns

5. Tree-shaking blocker in createFakerCore

As ST-DDT discovered in the PR comments, importing mergeLocales in createFakerCore pulls simpleFaker into the bundle via side effects. This partially undermines the tree-shaking benefit.

src/faker-core.ts:4-5:

import { mergeLocales } from './utils/merge-locales';
import { generateMersenne53Randomizer } from './utils/mersenne';

Suggestion: Consider lazy imports or making mergeLocales optional:

export function createFakerCore(options) {
  const { definitions, randomizer, config } = options;
  const resolved = Array.isArray(definitions)
    ? (await import('./utils/merge-locales')).mergeLocales(definitions)
    : (definitions ?? {});
  // ...
}

Or split into createFakerCore (simple, no mergeLocales) and createFakerCoreFromLocales (with merging).

6. Transform script fragility

The generate-module-tree.ts script uses string replacements for JSDoc transformation (src/scripts/generate-module-tree.ts:148-169):

.replace(' * @param fakerCore The FakerCore to use.\n', '')
.replaceAll(/ +\*\n +\*\n/g, ' *\n')
.replaceAll(new RegExp(`${methodName}\\(fakerCore(?:, ?)?`, 'g'), ...)

This is fragile - any deviation in JSDoc formatting (extra whitespace, different param descriptions) could cause silent failures. The restoreFakerTreeInvocations regex logic (lines 108-117) is also complex and could produce incorrect results with edge cases.

Suggestion: Consider using ts-morph's AST manipulation for JSDoc transformations instead of regex string replacements where possible. Also: should the transform scripts (tranform-once.ts) be deleted before merge as mentioned in the PR description?

7. The tranform-once.ts typo and code duplication

The filename has a typo: tranform-once.ts (missing 's' in "transform"). Also, it duplicates the case conversion utilities that also exist in scripts/shared/character-case.ts (lines 133-147 of the script vs the shared module).

Suggestion: Fix the typo if the file is being kept, and deduplicate the case conversion utilities. Per the PR description this script gets deleted before merge, so this may be moot.

8. bio() relying on fake() without explicit entrypoints

src/modules/person/bio.ts:21:

return fake(fakerCore, bio_pattern);

The bio_pattern locale data contains patterns like {{person.bio_part}}, {{internet.emoji}}, {{word.noun}}. Since fake() defaults to [fakerCore.definitions], these resolve as locale data paths - which works today. But this creates an implicit contract: any locale data pattern used without explicit entrypoints must only reference other locale data paths, never module functions.

If a locale contributor adds a pattern like {{person.firstName}} to bio_pattern, it would break standalone bio() while still working via faker.person.bio() (which uses moduleRegistry as entrypoint).

Suggestion: Add a comment in the standalone fake() JSDoc or a lint rule that flags standalone fake() calls without explicit entrypoints, or at least document this constraint for locale contributors.


Minor Issues / Suggestions

9. Inconsistent module naming in registries

Registry keys don't prefix with module name (good), but the import aliases do:

import { bear as animalBear } from './bear';
export const animalModule = {
  bear: animalBear,  // key is 'bear', not 'animalBear'
};

This is correct behavior, but the verbose aliasing adds visual noise. Since each registry file is in its own module directory, the aliasing isn't strictly necessary - a simpler approach would be:

import { bear } from './bear';
export const animalModule = { bear };

The aliasing is needed in index.ts (to avoid conflicts), but not in registry.ts. Though I understand this may be generated code, so simplifying the generator might be the better approach.

10. FakerConfig is very minimal

export interface FakerConfig {
  defaultRefDate?: () => Date;
}

For the POC this is fine, but worth thinking ahead about what else might land here (timeZone, locale preferences, etc.) to ensure the interface stays clean.

11. No type export for the module registry shape

The moduleRegistry object is exported but its type is inferred. Users doing advanced things (like wrapping or extending it) would benefit from an explicit type:

export type ModuleRegistry = typeof moduleRegistry;

12. JSDoc @since 10.4.0 on new APIs

createFakerCore is tagged @since 10.4.0. Make sure this version number is correct when merging.


Questions for Discussion

  1. Should standalone functions be public API in this PR, or should this land as internal-only first? The PR description mentions this is a POC. If we merge as internal-only, we can iterate on the API surface without breaking changes.

  2. How should we handle the fake() entrypoints divergence? Three options:

    • a) Standalone fake() defaults to [moduleRegistry, fakerCore.definitions] (matches class, kills tree-shaking for fake)
    • b) Keep the divergence but document clearly
    • c) Make fake() require explicit entrypoints (no default) to force awareness
  3. Per xDivisionByZerox's suggestion about splitting into smaller PRs per module - is there a plan to do this before merge? I agree the overall approach is solid, but reviewing 359 files at once is challenging. If we could merge the infrastructure (FakerCore, fakerToCore, module-registry, scripts) first, then add modules incrementally, the review quality would be higher.

  4. The mergeLocales tree-shaking issue - has a workaround been found for the side-effect that pulls simpleFaker into the bundle?


What Looks Good

  • The FakerCore interface is clean and minimal
  • The assertLocaleData() pattern is consistent and provides good error messages
  • Inline mini-registries for fake() calls (e.g., streetAddress, company.name, productDescription) are elegant and tree-shake-friendly
  • The _ prefix convention for internal helpers (_eval.ts, _luhn-check.ts, _char-mappings.ts, _select-definition.ts) is clear
  • Bundle size results are promising (~30% reduction)
  • The generate-module-tree script approach ensures consistency across all modules
  • All existing tests pass, and coverage actually increased slightly

TL;DR

The architecture is solid and the bundle size wins are real. The main concerns are:

  1. fake() behaves differently between standalone and class API (critical to document or fix)
  2. Missing public API exports make standalone functions unusable for end users
  3. fakerToCore bypasses locale proxy validation (DX trade-off)
  4. Private field access via @ts-expect-error is fragile

I'd recommend addressing items 1 and 4 before merge, and having a plan for items 2 and 3. The split-by-module approach @xDivisionByZerox suggested would also make the final review more thorough.

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 22, 2026

@Shinigami92 Sorry for the question, but is that your opinion/an analysis backed by you personally or just a copy and paste from Claude's analysis of this PR?

@Shinigami92
Copy link
Copy Markdown
Member

@Shinigami92 Sorry for the question, but is that your opinion/an analysis backed by you personally or just a copy and paste from Claude's analysis of this PR?

second ⚠️
It was a try to make a helpful review for a near 400 file-change PR to provide you with hopefully helpful infos or creative directions
please let me know if you don't want reviews like this, and/or if anything about this comment is specifically bad, so I can e.g. improve prompts next time

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 22, 2026

I'm fine with reviews from AI tools, just add a hint that this does or does not reflect your personal opinions.
Some people use AI to help with formulating their ideas/translating their responses/actually sprinkle their own analysises/opinions throughout the text, so explicitly stating your intentions with posting it helps to clarify that.

I'm aware of all/most of the points brought up by the review, thats why I asked for explicit opinions here:
#3748 (comment)

@Shinigami92
Copy link
Copy Markdown
Member

I'm fine with reviews from AI tools, just add a hint that this does or does not reflect your personal opinions. Some people use AI to help with formulating their ideas/translating their responses/actually sprinkle their own analysises/opinions throughout the text, so explicitly stating your intentions with posting it helps to clarify that.

I'm aware of all/most of the points brought up by the review, thats why I asked for explicit opinions here: #3748 (comment)

ah now I understand where your comment was coming from 👍
yeah you are right, I will keep care about this next time

very good that you are aware of almost everything claude discovered
for the comment, I think we need a human-meeting together with @xDivisionByZerox, because everything else would be to complex and comment-write-intensive

@ST-DDT
Copy link
Copy Markdown
Member Author

ST-DDT commented Mar 22, 2026

FYI: I updated the FakerCore PR:

It uses a slightly different implementation then the one used in this PR but both are generally compatible.
I can modify either PR depending on which variant your prefer.

This/SMF PR:

  • Faker -> fakerToCore() -> FakerCore
    export function fakerToCore(faker: SimpleFaker | Faker): FakerCore {
    return {
    definitions: faker instanceof Faker ? faker.rawDefinitions : {},
    // @ts-expect-error: access private member field
    randomizer: faker._randomizer,
    config: {
    // @ts-expect-error: access private member field
    defaultRefDate: faker._defaultRefDate,
    },
    };
    }

FakerCore PR:

  • faker.fakerCore permanent field in class

    faker/src/simple-faker.ts

    Lines 123 to 125 in 6dada60

    constructor(options?: FakerCoreOptions) {
    this.fakerCore = createFakerCore(options);
    }

@ST-DDT ST-DDT marked this pull request as ready for review March 23, 2026 13:57
@ST-DDT ST-DDT requested a review from a team as a code owner March 23, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: feature Request for new feature p: 1-normal Nothing urgent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants